teslams icon indicating copy to clipboard operation
teslams copied to clipboard

open to a possible API refactor?

Open mreinstein opened this issue 8 years ago • 4 comments

what I'd like to change:

  • expose a getToken(email, password, callbackFn)
  • don't store the token internally in the module. pass it in with each call. (e.g, get_charge_state( vid, token, cb )
  • remove the JSONBig module dependency. Rather than fixing the id field, use the id_s field, which tesla already includes for vehicles. One less dependency.
  • support promises (any function calls that omit a callback would return a promise)

if any/all of these changes would be welcome, I'd be happy to send some PRs.

One thing to note, this would be a significant API change. We'd have to mark this as 2.x to respect semver.

@hjespers thoughts?

mreinstein avatar Apr 24 '16 19:04 mreinstein

Open to this. It's due for refactoring. If backward compatibility is broken I would want to take the opportunity to add better error handling that is more like request() and to unbundled all the examples that have taken on a life of their own.

hjespers avatar Apr 24 '16 19:04 hjespers

@mreinstein A small note to say that the TeslaJS library does something similar for token handling.

mseminatore avatar Apr 24 '16 21:04 mseminatore

Agreed on removing JSONbig dependency. This was added long before tesla added the id_s field so it's time for it to go.

hjespers avatar Apr 25 '16 16:04 hjespers

Finally finished the token handling enhancement request. Not necessary to pass it into every call. I created at set_token() function that sets it once and uses it for the remainder of the session. Also added "token" as an optional parameter where ever username and password was used before. Calling any function which used to take username/email and password with a token will now set the token for use in all other functions just as they were before.

On the JSONbig dependancy I still would need it or "id" and "id_s" would not resolve to the same values no?

hjespers avatar Jan 18 '17 18:01 hjespers