cronofy / cronofy-node

Node wrapper for the Cronofy API
https://docs.cronofy.com/developers
MIT License
49 stars 23 forks source link

Hold client details and tokens #20

Closed Haziba closed 7 years ago

Haziba commented 7 years ago

Note to tester: None

AdriVanHoudt commented 7 years ago

What is the reason to store the access token in the cronofy module? Right now when I do calls for multiple accounts I have 1 cronofy entity and pass the token when I do the call. With this change I would need to create a new cronofy instance on every call

Haziba commented 7 years ago

A couple of reasons for this change are so we can hold the client's state so you don't have to pass it around your project, and because then you could initialise it with your configuration details and then just call the methods without having to worry about tokens and keys at a later date

AdriVanHoudt commented 7 years ago

In most (if not all) of my cases I do a single call and maybe that call for multiple accounts meaning that for me Cronofy.readEvents({ access_token }, callback) is more efficient than Cronofy({ access_token }).readEvents(options, callback) If I'm correct calling Cronofy() will redo the method building every time, which is just a waste imo

EDIT: also this is a breaking change and not a minor since existing implementations will break ;)

stephenbinns commented 7 years ago

Hi @AdriVanHoudt do you mind sharing the use case for passing in multiple accounts is this a batch operation or something you do in a transaction?

AdriVanHoudt commented 7 years ago

I have 1 batch operation to refresh tokens and in other cases just a readevents for 1 user for example. The batch thing would definitely suffer from this change. And even in just the default case it seems unnecessary to rebuild the whole methods map every time you want an instance from the module.

Haziba commented 7 years ago

Closing in favour of https://github.com/cronofy/cronofy-node/pull/23, which allows the holding of state and passing in of access_tokens to each method