From cbfb1b75903f3aba52061a77885e9ed3b4988166 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Tue, 22 Jun 2021 11:47:31 -0700 Subject: [PATCH 01/34] Fix importing shared bundles in ESM workers (#6492) --- .../core/integration-tests/test/output-formats.js | 2 +- packages/packagers/js/src/ScopeHoistingPackager.js | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/core/integration-tests/test/output-formats.js b/packages/core/integration-tests/test/output-formats.js index daaa3eecee0..fb77056433c 100644 --- a/packages/core/integration-tests/test/output-formats.js +++ b/packages/core/integration-tests/test/output-formats.js @@ -825,7 +825,7 @@ describe('output formats', function() { .find(b => !b.filePath.includes('async')); assert( workerBundleContents.includes( - `importScripts("./${path.basename(syncBundle.filePath)}")`, + `import "./${path.basename(syncBundle.filePath)}"`, ), ); assert( diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index d640efea2d7..36bc21e1420 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -1047,10 +1047,15 @@ ${code} let importScripts = ''; let bundles = this.bundleGraph.getReferencedBundles(this.bundle); for (let b of bundles) { - importScripts += `importScripts("${relativeBundlePath( - this.bundle, - b, - )}");\n`; + if (this.bundle.env.outputFormat === 'esmodule') { + // importScripts() is not allowed in native ES module workers. + importScripts += `import "${relativeBundlePath(this.bundle, b)}";\n`; + } else { + importScripts += `importScripts("${relativeBundlePath( + this.bundle, + b, + )}");\n`; + } } res += importScripts; From e66dc12f0257faa4395f903b67d03b627dc62016 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Tue, 22 Jun 2021 11:47:55 -0700 Subject: [PATCH 02/34] Docs and API consistency for bundles (#6491) --- .../bundlers/default/src/DefaultBundler.js | 14 +-- packages/core/core/src/dumpGraphToGraphViz.js | 2 +- packages/core/core/src/public/Bundle.js | 4 +- .../core/src/public/MutableBundleGraph.js | 16 ++-- packages/core/core/src/types.js | 2 +- packages/core/core/test/PublicBundle.test.js | 2 +- .../node_modules/parcel-bundler-test/index.js | 2 +- .../parcel-bundler-splitable/index.js | 4 +- .../core/integration-tests/test/javascript.js | 16 ++-- .../integration-tests/test/output-formats.js | 4 +- .../integration-tests/test/scope-hoisting.js | 14 +-- packages/core/types/index.js | 88 +++++++++++++++---- packages/namers/default/src/DefaultNamer.js | 10 +-- packages/reporters/dev-server/src/Server.js | 2 +- packages/runtimes/js/src/JSRuntime.js | 5 +- 15 files changed, 115 insertions(+), 70 deletions(-) diff --git a/packages/bundlers/default/src/DefaultBundler.js b/packages/bundlers/default/src/DefaultBundler.js index 8f5a753c49c..1acea9e4144 100644 --- a/packages/bundlers/default/src/DefaultBundler.js +++ b/packages/bundlers/default/src/DefaultBundler.js @@ -108,11 +108,10 @@ export default (new Bundler({ for (let asset of assets) { let bundle = bundleGraph.createBundle({ entryAsset: asset, - isEntry: + needsStableName: asset.bundleBehavior === 'inline' ? false : dependency.isEntry || dependency.needsStableName, - isInline: asset.bundleBehavior === 'inline', target: bundleGroup.target, }); bundleByType.set(bundle.type, bundle); @@ -165,12 +164,12 @@ export default (new Bundler({ env: asset.env, type: asset.type, target: bundleGroup.target, - isEntry: + needsStableName: asset.bundleBehavior === 'inline' || (dependency.priority === 'parallel' && !dependency.needsStableName) ? false - : parentBundle.isEntry, + : parentBundle.needsStableName, isInline: asset.bundleBehavior === 'inline', isSplittable: asset.isBundleSplittable ?? true, pipeline: asset.pipeline, @@ -231,7 +230,8 @@ export default (new Bundler({ // Don't add to BundleGroups for entry bundles, as that would require // another entry bundle depending on these conditions, making it difficult // to predict and reference. - !containingBundle.isEntry && + // TODO: reconsider this. This is only true for the global output format. + !containingBundle.needsStableName && !containingBundle.isInline && containingBundle.isSplittable, ); @@ -340,11 +340,13 @@ export default (new Bundler({ // Don't create shared bundles from entry bundles, as that would require // another entry bundle depending on these conditions, making it difficult // to predict and reference. + // TODO: reconsider this. This is only true for the global output format. + // This also currently affects other bundles with stable names, e.g. service workers. .filter(b => { let entries = b.getEntryAssets(); return ( - !b.isEntry && + !b.needsStableName && b.isSplittable && entries.every(entry => entry.id !== asset.id) ); diff --git a/packages/core/core/src/dumpGraphToGraphViz.js b/packages/core/core/src/dumpGraphToGraphViz.js index b79f8c92ac7..31415664546 100644 --- a/packages/core/core/src/dumpGraphToGraphViz.js +++ b/packages/core/core/src/dumpGraphToGraphViz.js @@ -117,7 +117,7 @@ export default async function dumpGraphToGraphViz( // $FlowFixMe } else if (node.type === 'bundle') { let parts = []; - if (node.value.isEntry) parts.push('entry'); + if (node.value.needsStableName) parts.push('stable name'); if (node.value.isInline) parts.push('inline'); if (parts.length) label += ' (' + parts.join(', ') + ')'; if (node.value.env) label += ` (${getEnvDescription(node.value.env)})`; diff --git a/packages/core/core/src/public/Bundle.js b/packages/core/core/src/public/Bundle.js index 8039c454718..5b4aca8b0d3 100644 --- a/packages/core/core/src/public/Bundle.js +++ b/packages/core/core/src/public/Bundle.js @@ -115,8 +115,8 @@ export class Bundle implements IBundle { return new Environment(this.#bundle.env); } - get isEntry(): ?boolean { - return this.#bundle.isEntry; + get needsStableName(): ?boolean { + return this.#bundle.needsStableName; } get isInline(): ?boolean { diff --git a/packages/core/core/src/public/MutableBundleGraph.js b/packages/core/core/src/public/MutableBundleGraph.js index fb6442ea2cf..ca1a8856412 100644 --- a/packages/core/core/src/public/MutableBundleGraph.js +++ b/packages/core/core/src/public/MutableBundleGraph.js @@ -156,7 +156,7 @@ export default class MutableBundleGraph extends BundleGraph let target = targetToInternalTarget(opts.target); let bundleId = hashString( 'bundle:' + - (opts.uniqueKey ?? nullthrows(entryAsset?.id)) + + (opts.entryAsset ? opts.entryAsset.id : opts.uniqueKey) + path.relative(this.#options.projectRoot, target.distDir), ); @@ -186,16 +186,20 @@ export default class MutableBundleGraph extends BundleGraph value: { id: bundleId, hashReference: HASH_REF_PREFIX + bundleId, - type: opts.type ?? nullthrows(entryAsset).type, + type: opts.entryAsset ? opts.entryAsset.type : opts.type, env: opts.env ? environmentToInternalEnvironment(opts.env) : nullthrows(entryAsset).env, entryAssetIds: entryAsset ? [entryAsset.id] : [], mainEntryId: entryAsset?.id, - pipeline: opts.pipeline ?? entryAsset?.pipeline, - isEntry: opts.isEntry, - isInline: opts.isInline, - isSplittable: opts.isSplittable ?? entryAsset?.isBundleSplittable, + pipeline: opts.entryAsset ? opts.entryAsset.pipeline : opts.pipeline, + needsStableName: opts.needsStableName, + isInline: opts.entryAsset + ? opts.entryAsset.bundleBehavior === 'inline' + : opts.isInline, + isSplittable: opts.entryAsset + ? opts.entryAsset.isBundleSplittable + : opts.isSplittable, isPlaceholder, target, name: null, diff --git a/packages/core/core/src/types.js b/packages/core/core/src/types.js index be2a4ea1a1d..2fddff0f9eb 100644 --- a/packages/core/core/src/types.js +++ b/packages/core/core/src/types.js @@ -434,7 +434,7 @@ export type Bundle = {| env: Environment, entryAssetIds: Array, mainEntryId: ?ContentKey, - isEntry: ?boolean, + needsStableName: ?boolean, isInline: ?boolean, isSplittable: ?boolean, isPlaceholder?: boolean, diff --git a/packages/core/core/test/PublicBundle.test.js b/packages/core/core/test/PublicBundle.test.js index 8fe446b494b..9037a557ff6 100644 --- a/packages/core/core/test/PublicBundle.test.js +++ b/packages/core/core/test/PublicBundle.test.js @@ -23,7 +23,7 @@ describe('Public Bundle', () => { displayName: null, publicId: null, pipeline: null, - isEntry: null, + needsStableName: null, isInline: null, isSplittable: true, target: { diff --git a/packages/core/integration-tests/test/integration/cache/node_modules/parcel-bundler-test/index.js b/packages/core/integration-tests/test/integration/cache/node_modules/parcel-bundler-test/index.js index 16b1e242eef..3e286b91ba1 100644 --- a/packages/core/integration-tests/test/integration/cache/node_modules/parcel-bundler-test/index.js +++ b/packages/core/integration-tests/test/integration/cache/node_modules/parcel-bundler-test/index.js @@ -15,7 +15,7 @@ module.exports = new Bundler({ for (let asset of assets) { let bundle = bundleGraph.createBundle({ entryAsset: asset, - isEntry: Boolean(dependency.isEntry), + needsStableName: Boolean(dependency.isEntry), target: bundleGroup.target, }); diff --git a/packages/core/integration-tests/test/integration/commonjs-bundle-require/node_modules/parcel-bundler-splitable/index.js b/packages/core/integration-tests/test/integration/commonjs-bundle-require/node_modules/parcel-bundler-splitable/index.js index 7d6b1008630..a024fce6434 100644 --- a/packages/core/integration-tests/test/integration/commonjs-bundle-require/node_modules/parcel-bundler-splitable/index.js +++ b/packages/core/integration-tests/test/integration/commonjs-bundle-require/node_modules/parcel-bundler-splitable/index.js @@ -67,7 +67,7 @@ var _default = new _plugin.Bundler({ for (let asset of assets) { let bundle = bundleGraph.createBundle({ entryAsset: asset, - isEntry: asset.isIsolated ? false : Boolean(dependency.isEntry), + needsStableName: asset.isIsolated ? false : Boolean(dependency.isEntry), isInline: asset.isInline, target: bundleGroup.target, }); @@ -133,7 +133,7 @@ var _default = new _plugin.Bundler({ let bundle = bundleGraph.createBundle({ entryAsset: asset, target: bundleGroup.target, - isEntry: bundleGroupDependency.isEntry, + needsStableName: bundleGroupDependency.isEntry, isInline: asset.isInline, }); bundleByType.set(bundle.type, bundle); diff --git a/packages/core/integration-tests/test/javascript.js b/packages/core/integration-tests/test/javascript.js index 0d330cebdf2..ed609cd21a6 100644 --- a/packages/core/integration-tests/test/javascript.js +++ b/packages/core/integration-tests/test/javascript.js @@ -425,7 +425,7 @@ describe('javascript', function() { path.join(__dirname, '/integration/dynamic-import-attributes/index.js'), ); - let mainBundle = b.getBundles().find(b => b.isEntry); + let mainBundle = b.getBundles()[0]; let mainBundleContent = await outputFS.readFile( mainBundle.filePath, 'utf8', @@ -1006,10 +1006,7 @@ describe('javascript', function() { }, ]); - let contents = await outputFS.readFile( - b.getBundles().find(b => b.isEntry).filePath, - 'utf8', - ); + let contents = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8'); assert(!contents.includes('import.meta.url')); }); @@ -1082,10 +1079,7 @@ describe('javascript', function() { }, ]); - let contents = await outputFS.readFile( - b.getBundles().find(b => b.isEntry).filePath, - 'utf8', - ); + let contents = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8'); assert(!contents.includes('import.meta.url')); }); @@ -3565,7 +3559,9 @@ describe('javascript', function() { ); let bundles = b.getBundles(); - let asyncJsBundles = bundles.filter(b => !b.isEntry && b.type === 'js'); + let asyncJsBundles = bundles.filter( + b => !b.needsStableName && b.type === 'js', + ); assert.equal(asyncJsBundles.length, 2); // Every bundlegroup with an async js bundle should have the corresponding css diff --git a/packages/core/integration-tests/test/output-formats.js b/packages/core/integration-tests/test/output-formats.js index fb77056433c..56938512123 100644 --- a/packages/core/integration-tests/test/output-formats.js +++ b/packages/core/integration-tests/test/output-formats.js @@ -659,7 +659,7 @@ describe('output formats', function() { ]); let dist = await outputFS.readFile( - b.getBundles().find(b => !b.isEntry).filePath, + b.getBundles().find(b => !b.needsStableName).filePath, 'utf8', ); assert(dist.includes('$parcel$interopDefault')); @@ -715,7 +715,7 @@ describe('output formats', function() { ); let async = await outputFS.readFile( - b.getChildBundles(b.getBundles().find(b => b.isEntry))[0].filePath, + b.getChildBundles(b.getBundles()[0])[0].filePath, 'utf8', ); assert(!async.includes('$import$')); diff --git a/packages/core/integration-tests/test/scope-hoisting.js b/packages/core/integration-tests/test/scope-hoisting.js index fe5ed36f46d..d57d227fd9b 100644 --- a/packages/core/integration-tests/test/scope-hoisting.js +++ b/packages/core/integration-tests/test/scope-hoisting.js @@ -4333,14 +4333,7 @@ describe('scope hoisting', function() { ), ); - let entryBundle; - b.traverseBundles((bundle, ctx, traversal) => { - if (bundle.isEntry) { - entryBundle = bundle; - traversal.stop(); - } - }); - let entryAsset = entryBundle.getMainEntry(); + let entryAsset = b.getBundles()[0].getMainEntry(); // TODO: this test doesn't currently work in older browsers since babel // replaces the typeof calls before we can get to them. @@ -5426,10 +5419,7 @@ describe('scope hoisting', function() { ), ); - let contents = await outputFS.readFile( - b.getBundles().find(bundle => bundle.isEntry).filePath, - 'utf8', - ); + let contents = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8'); assert(contents.includes('=>')); let calls = []; diff --git a/packages/core/types/index.js b/packages/core/types/index.js index 0b1cc309502..6e6f37d92c0 100644 --- a/packages/core/types/index.js +++ b/packages/core/types/index.js @@ -1046,27 +1046,43 @@ export type CreateBundleOpts = // If an entryAsset is provided, a bundle id, type, and environment will be // inferred from the entryAsset. | {| - +uniqueKey?: string, + /** The entry asset of the bundle. If provided, many bundle properties will be inferred from it. */ +entryAsset: Asset, + /** The target of the bundle. Should come from the dependency that created the bundle. */ +target: Target, - +isEntry?: ?boolean, - +isInline?: ?boolean, - +isSplittable?: ?boolean, - +type?: ?string, - +env?: ?Environment, - +pipeline?: ?string, + /** + * Indicates that the bundle's file name should be stable over time, even when the content of the bundle + * changes. This is useful for entries that a user would manually enter the URL for, as well as for things + * like service workers or RSS feeds, where the URL must remain consistent over time. + */ + +needsStableName?: ?boolean, |} // If an entryAsset is not provided, a bundle id, type, and environment must // be provided. | {| + /** The type of the bundle. */ + +type: string, + /** The environment of the bundle. */ + +env: Environment, + /** A unique value for the bundle to be used in its id. */ +uniqueKey: string, - +entryAsset?: Asset, + /** The target of the bundle. Should come from the dependency that created the bundle. */ +target: Target, - +isEntry?: ?boolean, + /** + * Indicates that the bundle's file name should be stable over time, even when the content of the bundle + * changes. This is useful for entries that a user would manually enter the URL for, as well as for things + * like service workers or RSS feeds, where the URL must remain consistent over time. + */ + +needsStableName?: ?boolean, + /** Whether this bundle should be inlined into the parent bundle(s), */ +isInline?: ?boolean, + /** + * Whether the bundle can be split. If false, then all dependencies of the bundle will be kept + * internal to the bundle, rather than referring to other bundles. This may result in assets + * being duplicated between multiple bundles, but can be useful for things like server side rendering. + */ +isSplittable?: ?boolean, - +type: string, - +env: Environment, + /** The bundle's pipeline, to be used for optimization. Usually based on the pipeline of the entry asset. */ +pipeline?: ?string, |}; @@ -1099,26 +1115,50 @@ export type ExportSymbolResolution = {| * @section bundler */ export interface Bundle { + /** The bundle id. */ +id: string; - /** Whether this value is inside filePath it will be replace with the real hash at the end. */ - +hashReference: string; + /** The type of the bundle. */ +type: string; + /** The environment of the bundle. */ +env: Environment; - /** Whether this is an entry (e.g. should not be hashed). */ - +isEntry: ?boolean; + /** The bundle's target. */ + +target: Target; + /** + * Indicates that the bundle's file name should be stable over time, even when the content of the bundle + * changes. This is useful for entries that a user would manually enter the URL for, as well as for things + * like service workers or RSS feeds, where the URL must remain consistent over time. + */ + +needsStableName: ?boolean; /** Whether this bundle should be inlined into the parent bundle(s), */ +isInline: ?boolean; + /** + * Whether the bundle can be split. If false, then all dependencies of the bundle will be kept + * internal to the bundle, rather than referring to other bundles. This may result in assets + * being duplicated between multiple bundles, but can be useful for things like server side rendering. + */ +isSplittable: ?boolean; - +target: Target; - /** Assets that run when the bundle is loaded (e.g. runtimes could be added). VERIFY */ + /** + * A placeholder for the bundle's content hash that can be used in the bundle's name or the contents of another + * bundle. Hash references are replaced with a content hash of the bundle after packaging and optimizing. + */ + +hashReference: string; + /** + * Returns the assets that are executed immediately when the bundle is loaded. + * Some bundles may not have any entry assets, for example, shared bundles. + */ getEntryAssets(): Array; - /** The actual entry (which won't be a runtime). */ + /** + * Returns the main entry of the bundle, which will provide the bundle's exports. + * Some bundles do not have a main entry, for example, shared bundles. + */ getMainEntry(): ?Asset; + /** Returns whether the bundle includes the given asset. */ hasAsset(Asset): boolean; + /** Returns whether the bundle includes the given dependency. */ hasDependency(Dependency): boolean; /** Traverses the assets in the bundle. */ traverseAssets(visit: GraphVisitor): ?TContext; - /** Traverses assets and dependencies (see BundleTraversable). */ + /** Traverses assets and dependencies in the bundle. */ traverse( visit: GraphVisitor, ): ?TContext; @@ -1129,13 +1169,21 @@ export interface Bundle { * @section bundler */ export interface NamedBundle extends Bundle { + /** A shortened version of the bundle id that is used to refer to the bundle at runtime. */ +publicId: string; + /** + * The bundle's name. This is a file path relative to the bundle's target directory. + * The bundle name may include a hash reference, but not the final content hash. + */ +name: string; + /** A version of the bundle's name with hash references removed for display. */ +displayName: string; } export interface PackagedBundle extends NamedBundle { + /** The absolute file path of the written bundle, including the final content hash if any. */ +filePath: FilePath; + /** Statistics about the bundle. */ +stats: Stats; } @@ -1144,7 +1192,9 @@ export interface PackagedBundle extends NamedBundle { * @section bundler */ export type BundleGroup = {| + /** The target of the bundle group. */ +target: Target, + /** The id of the entry asset in the bundle group, which is executed immediately when the bundle group is loaded. */ +entryAssetId: string, |}; diff --git a/packages/namers/default/src/DefaultNamer.js b/packages/namers/default/src/DefaultNamer.js index 43a00011879..b27cd01610d 100644 --- a/packages/namers/default/src/DefaultNamer.js +++ b/packages/namers/default/src/DefaultNamer.js @@ -15,9 +15,9 @@ export default (new Namer({ let bundleGroup = bundleGraph.getBundleGroupsContainingBundle(bundle)[0]; let bundleGroupBundles = bundleGraph.getBundlesInBundleGroup(bundleGroup); - if (bundle.isEntry) { + if (bundle.needsStableName) { let entryBundlesOfType = bundleGroupBundles.filter( - b => b.isEntry && b.type === bundle.type, + b => b.needsStableName && b.type === bundle.type, ); assert( entryBundlesOfType.length === 1, @@ -34,7 +34,7 @@ export default (new Namer({ if ( bundle.id === mainBundle.id && - bundle.isEntry && + bundle.needsStableName && bundle.target && bundle.target.distEntry != null ) { @@ -87,7 +87,7 @@ export default (new Namer({ bundleGroup.entryAssetId, options.entryRoot, ); - if (!bundle.isEntry) { + if (!bundle.needsStableName) { name += '.' + bundle.hashReference; } @@ -106,7 +106,7 @@ function nameFromContent( let name = basenameWithoutExtension(entryFilePath); // If this is an entry bundle, use the original relative path. - if (bundle.isEntry) { + if (bundle.needsStableName) { // Match name of target entry if possible, but with a different extension. if (bundle.target.distEntry != null) { return basenameWithoutExtension(bundle.target.distEntry); diff --git a/packages/reporters/dev-server/src/Server.js b/packages/reporters/dev-server/src/Server.js index 50a429eca4f..2df876de520 100644 --- a/packages/reporters/dev-server/src/Server.js +++ b/packages/reporters/dev-server/src/Server.js @@ -163,7 +163,7 @@ export default class Server { // If the main asset is an HTML file, serve it let htmlBundleFilePaths = []; this.bundleGraph.traverseBundles(bundle => { - if (bundle.type === 'html' && bundle.isEntry) { + if (bundle.type === 'html' && !bundle.isInline) { htmlBundleFilePaths.push(bundle.filePath); } }); diff --git a/packages/runtimes/js/src/JSRuntime.js b/packages/runtimes/js/src/JSRuntime.js index afdba4f7fe3..7f35a978b13 100644 --- a/packages/runtimes/js/src/JSRuntime.js +++ b/packages/runtimes/js/src/JSRuntime.js @@ -480,8 +480,11 @@ function isNewContext( bundleGraph: BundleGraph, ): boolean { let parents = bundleGraph.getParentBundles(bundle); + let isInEntryBundleGroup = bundleGraph + .getBundleGroupsContainingBundle(bundle) + .some(g => bundleGraph.isEntryBundleGroup(g)); return ( - bundle.isEntry || + isInEntryBundleGroup || parents.length === 0 || parents.some( parent => From 41a9a11ad4ab5ded8168684df9c9f84065bdaf50 Mon Sep 17 00:00:00 2001 From: Gora Kong <15333808+gorakong@users.noreply.github.com> Date: Wed, 23 Jun 2021 12:29:24 -0700 Subject: [PATCH 03/34] Prevent the creation of single-bundle shared bundles (#6403) * do not create a shared bundle from a single source bundle * added integration test * wip fix test * fix test * resolve conflicts and fix test * add lodash/difference to another bundle * fix test --- .../bundlers/default/src/DefaultBundler.js | 51 ++++++++----------- packages/core/core/src/ContentGraph.js | 2 +- packages/core/integration-tests/test/html.js | 4 +- .../shared-bundle-single-source/a.js | 9 ++++ .../shared-bundle-single-source/b.js | 7 +++ .../shared-bundle-single-source/bar.js | 6 +++ .../shared-bundle-single-source/foo.js | 4 ++ .../shared-bundle-single-source/index.js | 4 ++ .../shared-bundle-single-source/local.html | 6 +++ .../shared-bundle-single-source/package.json | 6 +++ .../shared-bundle-single-source/styles.css | 1 + .../shared-bundle-single-source/yarn.lock | 0 .../test/integration/shared-many/e.js | 1 + .../core/integration-tests/test/javascript.js | 40 +++++++++++++++ 14 files changed, 108 insertions(+), 33 deletions(-) create mode 100644 packages/core/integration-tests/test/integration/shared-bundle-single-source/a.js create mode 100644 packages/core/integration-tests/test/integration/shared-bundle-single-source/b.js create mode 100644 packages/core/integration-tests/test/integration/shared-bundle-single-source/bar.js create mode 100644 packages/core/integration-tests/test/integration/shared-bundle-single-source/foo.js create mode 100644 packages/core/integration-tests/test/integration/shared-bundle-single-source/index.js create mode 100644 packages/core/integration-tests/test/integration/shared-bundle-single-source/local.html create mode 100644 packages/core/integration-tests/test/integration/shared-bundle-single-source/package.json create mode 100644 packages/core/integration-tests/test/integration/shared-bundle-single-source/styles.css create mode 100644 packages/core/integration-tests/test/integration/shared-bundle-single-source/yarn.lock diff --git a/packages/bundlers/default/src/DefaultBundler.js b/packages/bundlers/default/src/DefaultBundler.js index 1acea9e4144..77582b855f0 100644 --- a/packages/bundlers/default/src/DefaultBundler.js +++ b/packages/bundlers/default/src/DefaultBundler.js @@ -388,31 +388,34 @@ export default (new Bundler({ .sort((a, b) => b.size - a.size); for (let {assets, sourceBundles} of sortedCandidates) { - // Find all bundle groups connected to the original bundles - let bundleGroups = new Set(); + let eligibleSourceBundles = new Set(); for (let bundle of sourceBundles) { - for (let bundleGroup of bundleGraph.getBundleGroupsContainingBundle( - bundle, - )) { - bundleGroups.add(bundleGroup); + // Find all bundle groups connected to the original bundles + let bundleGroups = bundleGraph.getBundleGroupsContainingBundle(bundle); + // Check if all bundle groups are within the parallel request limit + if ( + bundleGroups.every( + group => + bundleGraph + .getBundlesInBundleGroup(group) + .filter(b => !b.isInline).length < config.maxParallelRequests, + ) + ) { + eligibleSourceBundles.add(bundle); } } - // If all bundle groups have already met the max parallel request limit, then they cannot be split. - if ( - Array.from(bundleGroups).every( - group => - bundleGraph.getBundlesInBundleGroup(group).filter(b => !b.isInline) - .length >= config.maxParallelRequests, - ) - ) { + // Do not create a shared bundle unless there are at least 2 source bundles + if (eligibleSourceBundles.size < 2) { continue; } - let [firstBundle] = [...sourceBundles]; + let [firstBundle] = [...eligibleSourceBundles]; let sharedBundle = bundleGraph.createBundle({ - uniqueKey: hashString([...sourceBundles].map(b => b.id).join(':')), + uniqueKey: hashString( + [...eligibleSourceBundles].map(b => b.id).join(':'), + ), // Allow this bundle to be deduplicated. It shouldn't be further split. // TODO: Reconsider bundle/asset flags. isSplittable: true, @@ -426,20 +429,8 @@ export default (new Bundler({ for (let asset of assets) { bundleGraph.addAssetGraphToBundle(asset, sharedBundle); - for (let bundle of sourceBundles) { - // Remove the asset graph from the bundle if all bundle groups are - // within the parallel request limit and will include the shared bundle. - let bundleGroups = bundleGraph.getBundleGroupsContainingBundle( - bundle, - ); - if ( - bundleGroups.every( - bundleGroup => - bundleGraph - .getBundlesInBundleGroup(bundleGroup) - .filter(b => !b.isInline).length < config.maxParallelRequests, - ) - ) { + for (let bundle of eligibleSourceBundles) { + { bundleGraph.createBundleReference(bundle, sharedBundle); bundleGraph.removeAssetGraphFromBundle(asset, bundle); } diff --git a/packages/core/core/src/ContentGraph.js b/packages/core/core/src/ContentGraph.js index 57e9cc87b26..0be5dd1c6c0 100644 --- a/packages/core/core/src/ContentGraph.js +++ b/packages/core/core/src/ContentGraph.js @@ -68,7 +68,7 @@ export default class ContentGraph< getNodeIdByContentKey(contentKey: ContentKey): NodeId { return nullthrows( this._contentKeyToNodeId.get(contentKey), - 'Expected content key to exist', + `Expected content key ${contentKey} to exist`, ); } diff --git a/packages/core/integration-tests/test/html.js b/packages/core/integration-tests/test/html.js index c3ec4884c59..72dd9c113a2 100644 --- a/packages/core/integration-tests/test/html.js +++ b/packages/core/integration-tests/test/html.js @@ -1884,7 +1884,7 @@ describe('html', function() { }); let html = await outputFS.readFile(path.join(distDir, 'a.html'), 'utf8'); - assert.equal(html.match(/