search

MONEI / Shopify-api-node

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

Use a logical approach for resource modules (ie: stop using dynamic imports) #506

Closed panoply closed 11 months ago

panoply commented 3 years ago

Why do this? https://github.com/MONEI/Shopify-api-node/blob/master/resources/index.js

The implications for loading modules in this manner for exceed the benefits. Bundlers are unable to efficiently treeshake and those mapped dynamic requires wreak absolute havoc when one attempts to re-route them in transpilation. Please, move to ESM or are the very least process the project through a bundler that prevents that dynamic mapping.

lpinca commented 3 years ago

Why do this?

  1. The library was designed before "serverless" was a thing.
  2. The library was designed for server-side usage.
  3. Only pay for what you use. There is no need to load everything if it is not used.

To summarize, bundlers were different and considered useless for this library when it was designed.

panoply commented 3 years ago

Thanks for the fast response, really appreciate it.

Are there any plans to bring the project upto a modern standard? I would be happy to help but do not want to PR an overhaul without aligning with your ambitions for the project.

The library was designed before "serverless" was a thing.

Understood. Serverless implementation is not so much of an issue even with the dynamics currently in place. Providers like Netlify and those alike will generally provide a solution to combat that, ie: explicitly inferring the entire module through outside configs. This issue pertains to treeshaking more than anything.

If a redesign is something you would consider, again I would be happy to help.

For what it's worth, this module is an elegant drop-in solution. I prefer it over the Shopify maintained solutions in the nexus (which are typically horrible and are lucky to make it a year before deprecation).

Thanks.

lpinca commented 3 years ago

No plans at the moment. Lazy requires are the only thing users sometimes complain about due to bundling issues, so perhaps they can be removed in the next major version. We could also move to a pure ESM implementation but it is not a priority.

reecefenwick commented 11 months ago

Facing this issue as well, are there any implications with changing from lazy/dynamic imports?

I understand the rationale, but feel it could be an unnecessary optimisation. I'm working in large server side project with a large dependency tree and this is the only dependency I have that is not compatible for bundling

reecefenwick commented 11 months ago

I've raised https://github.com/MONEI/Shopify-api-node/pull/634 to fix this with backwards compatibility