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

Improve external and inter-chunk import execution order #2508

Merged
merged 7 commits into from Oct 28, 2018

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Oct 14, 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 (it is a breaking change if people were relying on a wrong execution order, though)

List any relevant issue numbers:
Resolves #2490

Description

Previously, the order of external imports as well as the order of imports from other chunks was determined via this simple algorithm:

  • determine the (correct) execution oder of modules inside the chunk
  • following the execution order, add all external dependencies of the modules to the chunk

If there are no cycles, this satisfied the guarantee that a module was only executed once all its dependencies had been executed. If external modules had side-effects, it was not guaranteed, though, that these side-effects were executed in order. Consider this example:

// main.js
import 'external1';
import './other.js';
...

// other.js
import 'external2';
...

Inside the chunk, the execution order is other.js -> main.js, so the old algorithm would generate the execution order

external2 -> external1 -> other.js -> main.js

while the correct order is

external1 -> external2 -> other.js -> main.js

This PR fixes this by

  1. generation execution order indices for both internal and external modules (previously, this was only done for internal modules)
  2. sorting the dependencies of a chunk according to their execution indices the same way modules inside the chunk are sorted

Beyond that, this also refactors and extracts some of the analysis code. For reviewing, the new logic is entirely contained in the commit "Sort external and inter-chunk imports according to execution order".

Also, for the first time, this PR adds functional tests with code-splitting! These tests are run via a special require function that knows all generated chunks and resolves accordingly. I plan on refining this logic within the 1.0 PR.

Note that it is impossible to always maintain correct execution order, even via this PR, when bundling. This PR, however, gets us as close as possible. A trivial example where it is impossible to have correct execution order is this:

// main.js
import './internal.js';
import 'external';
console.log('main');

// internal.js
console.log('internal');

// resulting bundle
import 'external';

console.log('internal');

console.log('main');

which has the execution order external -> internal.js -> main while the correct execution order would be internal.js -> external -> main. But this lies within the nature of bundling.

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.

Ausgezeichnet, das habe ich vorher übersehen.


export function analyzeModuleExecution(
entryModules: Module[],
generateChunkHashes: boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that these hashes are not the same as "chunk hashes" as chunk hashes should be content-based.

So we have two concept of chunk hash:

  1. The content-based hash of the inner chunk content with output options
  2. The graph colouring hash

I prefer to use chunkHash to refer to (1) as it is what most users would naively expect I believe.

So either colouringHash or graphHash or similar for this would be a more useful name I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

}

for (const dynamicModule of module.dynamicImportResolutions) {
if (!(dynamicModule.resolution instanceof Module)) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess dynamic imports to external modules are ok without an execIndex, although worth bearing this in mind from a data consistency point of view.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Will change to make sure all (external and internal) modules are initialized consistently in case their execIndex is never set.

@lukastaegert lukastaegert merged commit 273ac73 into master Oct 28, 2018
@lukastaegert
Copy link
Member Author

BTW your German is impeccable, that is to say Dein Deutsch ist makellos 😂

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.

Imports are incorrectly reordered
2 participants