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

Dynamic imports wrongly loads static import dependencies inside packages #9182

Closed
SachaG opened this Issue Oct 5, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@SachaG
Contributor

SachaG commented Oct 5, 2017

When you use import() to dynamically import a file, you would expect that all of that file's static imports would also be loaded dynamically. In other words, if I load this file dynamically:

import SuperHeavyJSPackage from 'super-heavy-js-package';

Then SuperHeavyJSPackage will not be part of my app bundle.

While this works fine inside the imports directory, it seems like it doesn't inside the packages directory. What's more, apparently imports will run even when they shouldn't (for example, inside an if (false) {...} block).

Here's a reproduction I created with help from @Discordius, who found the issue:

https://github.com/SachaG/dynamic-imports-bug/tree/package

And here's the same code running out of imports to compare:

https://github.com/SachaG/dynamic-imports-bug/tree/imports

It seems like the problem has been around since 1.5 as far as I can tell.

@benjamn

This comment has been minimized.

Member

benjamn commented Oct 6, 2017

This is definitely a bug!

I don't expect this to mean much to anyone but me, but I've tracked the problem down to this line, which is executed when a Meteor package tries to import an npm package from the application node_modules directory. Specifically, the mistake seems to be that the forDynamicImport parameter is ignored, so we lose the crucial information that HeavyComponent.jsx was imported dynamically.

benjamn added a commit that referenced this issue Oct 7, 2017

@Discordius

This comment has been minimized.

Discordius commented Oct 7, 2017

Just as a quick question so I can decide whether to implement a hot fix myself: Do you have any guess how long it will be until I might be able to pull a fix to this? (ideally via npm and not via forking the repo)

benjamn added a commit that referenced this issue Oct 8, 2017

Merge pull request #9187 from meteor/refactor-peer-dependency-process…
…ing-and-fix-issue-9182

Refactor peer dependency processing and fix issue #9182.
@benjamn

This comment has been minimized.

Member

benjamn commented Oct 10, 2017

@Discordius You can try it now with Meteor 1.6: meteor update --release 1.6-rc.6. If you're not ready for that, we will probably back-port this to Meteor 1.5.3 (not yet released).

@benjamn benjamn closed this Oct 12, 2017

abernix added a commit that referenced this issue Oct 26, 2017

abernix added a commit that referenced this issue Oct 26, 2017

@abernix abernix referenced this issue Oct 28, 2017

Merged

Release 1.5.3 #9266

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment