Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
panoply opened this issue Aug 26, 2021 · 5 comments · Fixed by #634
Closed

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

panoply opened this issue Aug 26, 2021 · 5 comments · Fixed by #634

Comments

@panoply
Copy link

panoply commented Aug 26, 2021

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
Copy link
Collaborator

lpinca commented Aug 27, 2021

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
Copy link
Author

panoply commented Aug 27, 2021

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
Copy link
Collaborator

lpinca commented Aug 27, 2021

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
Copy link
Contributor

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
Copy link
Contributor

I've raised #634 to fix this with backwards compatibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants