Skip to content

Improve external and inter-chunk import execution order - #2508

Merged
lukastaegert merged 7 commits into
masterfrom
external-execution-order
Oct 28, 2018
Merged

Improve external and inter-chunk import execution order#2508
lukastaegert merged 7 commits into
masterfrom
external-execution-order

Conversation

@lukastaegert

@lukastaegert lukastaegert commented Oct 14, 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 (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.

@lukastaegert
lukastaegert force-pushed the external-execution-order branch from df0caff to dbbd52d Compare October 15, 2018 06:20

@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.

Ausgezeichnet, das habe ich vorher übersehen.

Comment thread src/utils/execution-order.ts Outdated

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

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.

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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