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

Inline deep dependencies for entry points #2073

Merged
merged 2 commits into from
Mar 23, 2018

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Mar 20, 2018

As described in #2071, this will minimize the latency waterfall problem for deep chunkings chains, making code splitting workflows faster.

Note this will likely have merge conflicts with #2068. Either can be merged first and I'll fix up the conflicts for the other.

@guybedford guybedford requested a review from lukastaegert March 20, 2018 16:09
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

That's a fantastic improvement, great idea!

Two things:

  • I added a minor suggestion but it is more cosmetic than anything
  • I think that this should preserve execution order between modules but since you are definitely the expert here, just wondering if you are sure and have checked this

this.imports.forEach(impt => {
if (this.dependencies.indexOf(impt.module) === -1) this.dependencies.push(impt.module);
});
}
Copy link
Member

Choose a reason for hiding this comment

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

As this if-else is basically a preparation step for getChunkDependencyDeclarations with the sole purpose of adding some additional dependencies to this.dependencies, I would suggest extracting it into a separate private method, e.g. addAdditionalDependencies or addDerivedDependencies or whatever rings best with you.
Then I would suggest to call this method not from within getChunkDependencyDeclarations but directly from getModuleDeclarations. Thus it is immediately visible that this public method has a side-effect on a chunk's dependencies (which may or may not be helpful for future refinements)

@guybedford
Copy link
Contributor Author

Thanks for review here - I've included the change.

Yes, execution order will definitely remain invariant with this change!

For example:

a.js

import 'b';

b.js

import 'c';
import 'd';
import 'e';

Can have a.js rewritten as import 'b'; import 'c'; import 'd'; import 'e'; or import 'b'; import 'e'; import 'd'; import 'c'; and will retain the same execution order as what matters is the first-visited pre-order traversal only.

@lukastaegert
Copy link
Member

as what matters is the first-visited pre-order traversal only.

💡Ah, that makes sense, thanks!

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

👍

@lukastaegert lukastaegert added this to the 0.58.0 milestone Mar 23, 2018
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.

2 participants