Shopify-api-node icon indicating copy to clipboard operation
Shopify-api-node copied to clipboard

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

Open panoply opened this issue 2 years ago • 3 comments

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.

panoply avatar Aug 26 '21 23:08 panoply

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.

lpinca avatar Aug 27 '21 05:08 lpinca

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.

panoply avatar Aug 27 '21 06:08 panoply

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.

lpinca avatar Aug 27 '21 06:08 lpinca

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 avatar Oct 09 '23 06:10 reecefenwick

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

reecefenwick avatar Oct 09 '23 07:10 reecefenwick