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

Thoroughly improve import resolution #2584

Merged
merged 6 commits into from Dec 11, 2018
Merged

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Dec 6, 2018

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!

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Great work!

@@ -9,7 +9,7 @@ define(['exports'], function (exports) { 'use strict';
});

exports.a = a;
exports.b = b;
Copy link
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
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
Contributor

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

@guybedford
Copy link
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
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
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
Member Author

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

See #2587

@lukastaegert lukastaegert added this to In progress in Release 1.0.0 via automation Dec 10, 2018
@lukastaegert lukastaegert moved this from In progress to In Review in Release 1.0.0 Dec 10, 2018
@lukastaegert lukastaegert changed the base branch from refactor-chunking to master December 11, 2018 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Release 1.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

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