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

Fix lazy compilation #9093

Merged
merged 6 commits into from
Jul 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/core/core/src/AssetGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -353,10 +353,12 @@ export default class AssetGraph extends ContentGraph<AssetGraphNode> {
nodeId !== traversedNodeId
) {
if (!ctx?.hasDeferred) {
this.safeToIncrementallyBundle = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do any of the other cases here need this added?

The symptoms I saw was that while the asset graph was correctly updated after a previously lazy bundle was requested, the bundle graph was not - so I've focused on cases where dependency nodes have changed from deferred to not deferred, rather than asset nodes.

delete traversedNode.hasDeferred;
}
actions.skipChildren();
} else if (traversedNode.type === 'dependency') {
this.safeToIncrementallyBundle = false;
traversedNode.hasDeferred = false;
} else if (nodeId !== traversedNodeId) {
actions.skipChildren();
Expand Down
4 changes: 4 additions & 0 deletions packages/core/core/src/BundleGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -1936,6 +1936,10 @@ export default class BundleGraph {
bundle.id + bundle.target.publicUrl + this.getContentHash(bundle),
);

if (bundle.isPlaceholder) {
hash.writeString('placeholder');
}

let inlineBundles = this.getInlineBundles(bundle);
for (let inlineBundle of inlineBundles) {
hash.writeString(this.getContentHash(inlineBundle));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export default () => {
return Promise.all([
import('./uses-static-component').then(c => {
return c.default()();
}),
import('./uses-static-component-async').then(c => {
return c.default();
}).then(s => {
return s();
})]);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Document</title>
</head>
<body>
<script src="./index.js" type="module"></script>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
async function main() {
const m = await import('./lazy-1');
await import('./parallel-lazy-1');
return m.default();
}

main();
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default async () => {
const { world } = await import('./lazy-2');
return `Hello ${world}`;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const world = 'world';
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"private": true
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default async () => {
const m = await import('./parallel-lazy-2');
return m.default;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default 'parallel lazy 2';
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => "static component";
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default async () => {
const m = await import('./static-component');
return m.default;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import staticComponent from "./static-component"
export default () => {
return staticComponent;
}
123 changes: 123 additions & 0 deletions packages/core/integration-tests/test/lazy-compile.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
import assert from 'assert';
import path from 'path';
import {
bundler,
outputFS,
distDir,
getNextBuild,
assertBundles,
removeDistDirectory,
run,
} from '@parcel/test-utils';

const findBundle = (bundleGraph, nameRegex) => {
return bundleGraph.getBundles().find(b => nameRegex.test(b.name));
};

const distDirIncludes = async matches => {
const files = await outputFS.readdir(distDir);
for (const match of matches) {
if (typeof match === 'string') {
if (!files.some(file => file === match)) {
throw new Error(
`No file matching ${match} was found in ${files.join(', ')}`,
);
}
} else {
if (!files.some(file => match.test(file))) {
throw new Error(
`No file matching ${match} was found in ${files.join(', ')}`,
);
}
}
}
return true;
};

describe('lazy compile', function () {
it('should lazy compile', async function () {
const b = await bundler(
path.join(__dirname, '/integration/lazy-compile/index.js'),
{
shouldBuildLazily: true,
mode: 'development',
shouldContentHash: false,
},
);

await removeDistDirectory();

const subscription = await b.watch();
let result = await getNextBuild(b);

// This simulates what happens if index.js is loaded as well as lazy-1 which loads lazy-2.
// While parallel-lazy-1 is also async imported by index.js, we pretend it wasn't requested (i.e. like
// if it was behind a different trigger).
result = await result.requestBundle(
findBundle(result.bundleGraph, /index.js/),
);
result = await result.requestBundle(
findBundle(result.bundleGraph, /^lazy-1/),
);
result = await result.requestBundle(
findBundle(result.bundleGraph, /^lazy-2/),
);

// Expect the bundle graph to contain the whole nest of lazy from `lazy-1`, but not
// `parallel-lazy-1` which wasn't requested.
assertBundles(result.bundleGraph, [
{
assets: ['index.js', 'bundle-url.js', 'cacheLoader.js', 'js-loader.js'],
},
{
assets: ['lazy-1.js', 'esmodule-helpers.js'],
},
{
assets: ['lazy-2.js'],
},
{
assets: ['parallel-lazy-1.js'],
},
]);

subscription.unsubscribe();

// Ensure the files match the bundle graph - lazy-2 should've been produced as it was requested
assert(await distDirIncludes(['index.js', /^lazy-1\./, /^lazy-2\./]));
});

it('should lazy compile properly when same module is used sync/async', async () => {
Copy link
Contributor Author

@marcins marcins Jul 24, 2023

Choose a reason for hiding this comment

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

This test still fails on this branch, it has been pulled in as an intentional broken test (hence the draft PR).

The "fix" is to remove the conditions for shouldBuildLazily in JSRuntime (e.g.:

// Also do this when building lazily or the runtime itself could get deduplicated and only
// exist in the parent. This causes errors if an old version of the parent without the runtime
// is already loaded.
if (
bundle.env.outputFormat === 'commonjs' ||
bundle.env.isLibrary ||
options.shouldBuildLazily
) {
externalBundles = [mainBundle];
} else {
)

With the code as it is, the whole bundle group doesn't get loaded - so when the uses-static-component-async bundle loads, it doesn't trigger a load of the static-component bundle, so it can't resolve the module.

I know that the"fix" (seen here: marcins@a36a37d) means that there is a potential issue when a bundle group grows after lazy compilation - this means Parcel can get into a state where it requires a browser refresh in order to recover. However, with the current state, a refresh won't heal as the load order is just incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed in the Core meeting, the "fix" for this has now been cherry-picked onto this branch. I've updated the comments to leave behind some trace that there was lazy build specific code previously there.

const b = await bundler(
path.join(__dirname, '/integration/lazy-compile/index-sync-async.js'),
{
shouldBuildLazily: true,
mode: 'development',
shouldContentHash: false,
},
);

await removeDistDirectory();

const subscription = await b.watch();
let result = await getNextBuild(b);
result = await result.requestBundle(
findBundle(result.bundleGraph, /^index-sync-async\./),
);
result = await result.requestBundle(
findBundle(result.bundleGraph, /^uses-static-component\./),
);
result = await result.requestBundle(
findBundle(result.bundleGraph, /^uses-static-component-async\./),
);
result = await result.requestBundle(
findBundle(result.bundleGraph, /^static-component\./),
);

let output = await run(result.bundleGraph);
assert.deepEqual(await output.default(), [
'static component',
'static component',
]);
subscription.unsubscribe();
});
});
19 changes: 10 additions & 9 deletions packages/runtimes/js/src/JSRuntime.js
Original file line number Diff line number Diff line change
Expand Up @@ -357,14 +357,13 @@ function getLoaderRuntime({
// Importing of the other bundles will be handled by the bundle group entry.
// Do the same thing in library mode for ES modules, as we are building for another bundler
// and the imports for sibling bundles will be in the target bundle.
// Also do this when building lazily or the runtime itself could get deduplicated and only
// exist in the parent. This causes errors if an old version of the parent without the runtime
// is already loaded.
if (
bundle.env.outputFormat === 'commonjs' ||
bundle.env.isLibrary ||
options.shouldBuildLazily
) {

// Previously we also did this when building lazily, however it seemed to cause issues in some cases.
// The original comment as to why is left here, in case a future traveller is trying to fix that issue:
// > [...] the runtime itself could get deduplicated and only exist in the parent. This causes errors if an
// > old version of the parent without the runtime
// > is already loaded.
if (bundle.env.outputFormat === 'commonjs' || bundle.env.isLibrary) {
externalBundles = [mainBundle];
} else {
// Otherwise, load the bundle group entry after the others.
Expand Down Expand Up @@ -443,7 +442,9 @@ function getLoaderRuntime({
loaderModules.push(code);
}

if (bundle.env.context === 'browser' && !options.shouldBuildLazily) {
// Similar to the comment above, this also used to be skipped when shouldBuildLazily was true,
// however it caused issues where a bundle group contained multiple bundles.
if (bundle.env.context === 'browser') {
loaderModules.push(
...externalBundles
// TODO: Allow css to preload resources as well
Expand Down