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

Sort the connections back into topological order after traversing runtimes #75

Merged
merged 3 commits into from
Sep 16, 2024
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
49 changes: 42 additions & 7 deletions packages/core/core/src/applyRuntimes.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import createAssetGraphRequest from './requests/AssetGraphRequest';
import {createDevDependency, runDevDepRequest} from './requests/DevDepRequest';
import {toProjectPath, fromProjectPathRelative} from './projectPath';
import {tracer, PluginTracer} from '@atlaspack/profiler';
import {DefaultMap} from '@atlaspack/utils';

type RuntimeConnection = {|
bundle: InternalBundle,
Expand Down Expand Up @@ -60,6 +61,35 @@ function nameRuntimeBundle(
bundle.displayName = name.replace(hashReference, '[hash]');
}

/**
* The applyRuntimes function is responsible for generating all the runtimes
* (assets created during the build that don't actually exist on disk) and then
* linking them into the bundle graph.
*
* Usually, the assets returned from a runtime will go in the same bundle. It is
* possible though, for a runtime to return an asset with a `parallel` priority,
* which allows it to be moved to a separate bundle. In practice, this is
* usually used to generate application manifest files.
*
* When adding a manifest bundle (a whole new separate bundle) during a runtime,
* it needs to be added to a bundle group which will be potentially referenced
* by another bundle group. To avoid trying to reference a manifest entry which
* hasn't been created yet, we process the bundles from the bottom up, so that
* children will always be available when parents try to reference them.
*
* However, when merging those connections in to the bundle graph, the reversed
* order can create a situation where the child bundles thought they were coming
* first and so take responsibility for loading in shared bundles. When the
* parents actually load first, they're expecting bundles to be loaded which
* aren't yet, creating module not found errors.
*
* To fix that, we restore the forward topological order once all the
* connections are created.
*
* Introduction of manifest bundles: https://github.com/parcel-bundler/parcel/pull/8837
* Introduction of reverse topology: https://github.com/parcel-bundler/parcel/pull/8981
*/

export default async function applyRuntimes<TResult: RequestResult>({
bundleGraph,
config,
Expand All @@ -82,18 +112,17 @@ export default async function applyRuntimes<TResult: RequestResult>({
configs: Map<string, Config>,
|}): Promise<Map<string, Asset>> {
let runtimes = await config.getRuntimes();
let connections: Array<RuntimeConnection> = [];

// As manifest bundles may be added during runtimes we process them in reverse topological
// sort order. This allows bundles to be added to their bundle groups before they are referenced
// by other bundle groups by loader runtimes
// Sort bundles into reverse topological order
Copy link
Contributor

Choose a reason for hiding this comment

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

This is topological order, not reverse

let bundles = [];
bundleGraph.traverseBundles({
exit(bundle) {
bundles.push(bundle);
},
});

let connectionMap = new DefaultMap(() => []);

for (let bundle of bundles) {
for (let runtime of runtimes) {
let measurement;
Expand Down Expand Up @@ -172,7 +201,7 @@ export default async function applyRuntimes<TResult: RequestResult>({
nameRuntimeBundle(connectionBundle, bundle);
}

connections.push({
connectionMap.get(connectionBundle).push({
bundle: connectionBundle,
assetGroup,
dependency,
Expand All @@ -192,8 +221,14 @@ export default async function applyRuntimes<TResult: RequestResult>({
}
}

// Correct connection order after generating runtimes in reverse order
connections.reverse();
// Sort the bundles back into forward topological order
Copy link
Contributor

Choose a reason for hiding this comment

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

You're now going for reverse topological order

let connections: Array<RuntimeConnection> = [];

bundleGraph.traverseBundles({
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't reverse topological order, I don't think

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be

const connections = [];
bundleGraph.traverseBundles({
    exit(bundle) {
        connections.push(...connectionMap.get(bundle));
    }
});
connections.reverse();

enter(bundle) {
connections.push(...connectionMap.get(bundle));
},
});

// Add dev deps for runtime plugins AFTER running them, to account for lazy require().
for (let runtime of runtimes) {
Expand Down
8 changes: 7 additions & 1 deletion packages/core/integration-tests/test/bundler.js
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,12 @@ describe.v2('bundler', function () {
assets: ['b.html'],
},
{
assets: ['a.js', 'cacheLoader.js', 'js-loader.js'],
assets: [
'a.js',
'bundle-manifest.js',
'cacheLoader.js',
'js-loader.js',
],
},
{
assets: ['bundle-manifest.js', 'bundle-url.js'], // manifest bundle
Expand All @@ -881,6 +886,7 @@ describe.v2('bundler', function () {
'cacheLoader.js',
'js-loader.js',
'esmodule-helpers.js',
'bundle-manifest.js',
],
},
{
Expand Down
1 change: 1 addition & 0 deletions packages/core/integration-tests/test/workers.js
Original file line number Diff line number Diff line change
Expand Up @@ -1254,6 +1254,7 @@ describe('atlaspack', function () {
'get-worker-url.js',
'lodash.js',
'esmodule-helpers.js',
'bundle-url.js',
],
},
{
Expand Down