Skip to content

Thoroughly improve import resolution - #2584

Merged
lukastaegert merged 6 commits into
masterfrom
better-import-resolution
Dec 11, 2018
Merged

Thoroughly improve import resolution#2584
lukastaegert merged 6 commits into
masterfrom
better-import-resolution

Conversation

@lukastaegert

@lukastaegert lukastaegert commented Dec 6, 2018

Copy link
Copy Markdown
Member

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
resolves #2576
resolves #2231
and quite possible a few more

Description

This is a huge follow-up to #2575. The original intention was to fix handling for dynamic namespaces when preserving modules which lead me down a deep rabbit whole and had me refactoring many core parts of the logic. This is what has been fixed and improved:

  • Preserve modules now supports dynamic namespaces which is done not by creating namespace objects but by using a native way of doing a namespace import depending on the output format (i.e. import * as lib from ... for esm, const lib = require(...) for cjs and so on). Note that namespace freezing will not be performed when preserving modules!
  • Shimming missing exports has been thoroughly re-worked and should now work properly for all export formats. Moreover, the shim variable will now always be deconflicted from existing variables of the same name!
  • The SystemJS rendering code has been refactored and partially cleaned up to support proper missing export shimming.
  • Export tracing has been thoroughly refactored. Most notably, there is no longer any tracing logic at all on chunk level! This was achieved by attaching the exporting module to all variables which also leads to better and more direct imports in some cases.
  • The imports of a module are now directly determined during the tree-shaking phase with the awesome benefit that no unnecessary imports are rendered any longer! E.g if a.js and b.js both import a variable x from c.js that is only used in one of them, the other will no longer render the import! The effect becomes more pronounced when handling namespace imports. And the new logic is in fact a little simpler than the old one!

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

});

exports.a = a;
exports.b = b;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, did you implement the import pruning!? Very cool. That closes #2231 if so.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I did :) and not by adding a final check but by not adding the unused imports in the first place.

@guybedford

Copy link
Copy Markdown
Contributor

Wondering as well if this has affected #2556 at all?

@guybedford

Copy link
Copy Markdown
Contributor

This was achieved by attaching the exporting module to all variables which also leads to better and more direct imports in some cases.

One thing to ensure here is that execution ordering is carefully maintained. Previously I remember looking into doing these "direct" imports but found that was causing chunk ordering to be changed in some cases (say chunkA imports X from chunkB which is just reexported from chunkC, it might short-circuit chunkB, and lose the fact that chunkB was supposed to execute).

I don't know if the above applies, but it is worth checking.

@lukastaegert

Copy link
Copy Markdown
Member Author

Thanks for the great reviews! I will check into everything soon. Special thanks for the hint about the execution order. The logic was already there to sort-of maintain execution order by making sure all direct dependencies are added event though nothing was imported from those files but it was broken because the dependency sorting was happening too early. Already added a commit that fixes this.

@lukastaegert

Copy link
Copy Markdown
Member Author

Wondering as well if this has affected #2556 at all?

Unfortunately not, this is a very separate and probably tricky issue. The question is what the correct SystemJS code should be in such a situation but this should probably be discussed in #2556

@lukastaegert
lukastaegert force-pushed the better-import-resolution branch 3 times, most recently from cda48e7 to 57f62b7 Compare December 7, 2018 14:12
@lukastaegert

Copy link
Copy Markdown
Member Author

Wondering as well if this has affected #2556 at all?

See #2587

@lukastaegert
lukastaegert changed the base branch from refactor-chunking to master December 11, 2018 06:52
variables into modules that actually use them. Get rid of variable
tracing logic on chunks.
Also, do not inline the execution list when preserving modules
and sorting only once all dependencies have been added
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 this pull request may close these issues.

experimentalPreserveModules opotion doesn't work with import * Chunk import pruning

2 participants