search

MONEI / Shopify-api-node

Node Shopify connector sponsored by MONEI
https://monei.com/shopify-payment-gateway/
MIT License
947 stars 278 forks source link

Add support for passing request lifecycle hooks down to got #560

Closed airhorns closed 1 year ago

airhorns commented 1 year ago

got has rich support for request lifecycle hooks that allow userland code to understand what is happening within got and to modify it's behaviour. shopify-api-node uses got hooks for some retry implementation, but I think that users of shopify-api-node like me also want to know about whats happening during retries and errors.

For this reason, I think that shopify-api-node should accept request hooks too! This allows users of shopify-api-node to log each retry, or decorate errors with extra context, or add in tracing and other observability hooks to get great visibility on what the library is doing. See a good example of this in the readme.

There's not much maintenance burden for this library if we just passthrough to got (like we do with the request options itself).

coveralls commented 1 year ago

Coverage Status

Coverage remained the same at 100.0% when pulling a1f0b177d04d4c12a0e668ba3b7e0636f6774830 on gadget-inc:got-request-hooks into fc41cf4ab92ef1a8cdcc1da71ded7425b562c9e6 on MONEI:master.

lpinca commented 1 year ago

I'm not a fan of this. It makes the library too coupled with the got dependency. If users start using this feature, it will be hard to change got for something else if we ever need to do it. Similarly, it will be hard to update got if the hook mechanism will change in future versions of got.

airhorns commented 1 year ago

Ok, do you like the idea of injectable hooks of any sort though?

I am ok to not couple to got, but I also think that we're unlikely to come up with a far better hook interface than they have already. I get wanting to preserve optionality about the implementation, but how likely do you think swapping out got really is, and if you want to swap out got for a different library, is it really that likely to be a minor version that doesn't involve other interface changes? I think as a user of shopify-api-node, if a major change happened under the hood for how requests were made, I'd be ok needing to update the way I passed in hooks to a new format. Also notable is that newer versions of got use ESM, so this library would have to update to use ESM as well in order to upgrade got major versions.

That said, we can invent a different interface for accepting the hooks, and then just pass them along to Got in the shape they are expecting. What interface for accepting hooks would feel right to you?

lpinca commented 1 year ago

Let me think about it. High level API wrappers usually do not provide this feature.

how likely do you think swapping out got really is

Unlikely but not impossible. I've thought about using fetch several times in the past.

and if you want to swap out got for a different library, is it really that likely to be a minor version that doesn't involve other interface changes?

We would need a major version because of got errors but that is a small breaking change compared to this.

Also notable is that newer versions of got use ESM, so this library would have to update to use ESM as well in order to upgrade got major versions.

I know, this is the reason why got@11 is used.

lpinca commented 1 year ago

Landed as https://github.com/MONEI/Shopify-api-node/commit/eeda19880b634d5080e3ab2cc5b77513276c8dbe.

airhorns commented 1 year ago

Thanks @lpinca