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

Allow parallel type change bundles to be reused by async siblings #9504

Merged
merged 6 commits into from
Feb 10, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
384 changes: 71 additions & 313 deletions packages/bundlers/default/src/DefaultBundler.js

Large diffs are not rendered by default.

7 changes: 4 additions & 3 deletions packages/core/core/src/BundleGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -1137,6 +1137,7 @@ export default class BundleGraph {
? firstAsset
: // Otherwise, find the first asset that belongs to this bundle.
assets.find(asset => this.bundleHasAsset(bundle, asset)) ||
assets.find(a => a.type === bundle.type) ||
Copy link
Member Author

Choose a reason for hiding this comment

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

fixes a separate bug that was exposed by these changes: resolving a dependency to an asset should try to resolve to an asset of the same type as the bundle it's in. without this, there was a crash in the JS packager that resolved an dependency to a CSS asset.

firstAsset;

// If a resolution still hasn't been found, return the first referenced asset.
Expand Down Expand Up @@ -1735,7 +1736,7 @@ export default class BundleGraph {
);
let depSymbol = symbolLookup.get(identifier);
if (depSymbol != null) {
let resolved = this.getResolvedAsset(dep);
let resolved = this.getResolvedAsset(dep, boundary);
if (!resolved || resolved.id === asset.id) {
// External module or self-reference
return {
Expand Down Expand Up @@ -1785,7 +1786,7 @@ export default class BundleGraph {
depSymbols.get('*')?.local === '*' &&
symbol !== 'default'
) {
let resolved = this.getResolvedAsset(dep);
let resolved = this.getResolvedAsset(dep, boundary);
if (!resolved) {
continue;
}
Expand Down Expand Up @@ -1918,7 +1919,7 @@ export default class BundleGraph {
if (!depSymbols) continue;

if (depSymbols.get('*')?.local === '*') {
let resolved = this.getResolvedAsset(dep);
let resolved = this.getResolvedAsset(dep, boundary);
if (!resolved) continue;
let exported = this.getExportedSymbols(resolved, boundary)
.filter(s => s.exportSymbol !== 'default')
Expand Down
47 changes: 46 additions & 1 deletion packages/core/integration-tests/test/bundler.js
Original file line number Diff line number Diff line change
Expand Up @@ -1524,7 +1524,7 @@ describe('bundler', function () {
]);

let targetDistDir = normalizePath(path.join(__dirname, '../dist'));
let hashedIdWithMSB = hashString('bundle:' + 'vendorjs' + targetDistDir);
let hashedIdWithMSB = hashString('bundle:' + 'vendor,js' + targetDistDir);
assert(
b.getBundles().find(b => b.id == hashedIdWithMSB),
'MSB id does not match expected',
Expand Down Expand Up @@ -1760,4 +1760,49 @@ describe('bundler', function () {
]);
});
});

it('should reuse type change bundles from parent bundle groups', async function () {
await fsFixture(overlayFS, __dirname)`
reuse-type-change-bundles
index.html:
<link rel="stylesheet" type="text/css" href="./style.css">
<script src="./index.js" type="module"></script>

style.css:
@import "common.css";
body { color: red }

common.css:
.common { color: green }

index.js:
import('./async');

async.js:
import './common.css';
`;

let b = await bundle(
path.join(__dirname, 'reuse-type-change-bundles', 'index.html'),
{
mode: 'production',
inputFS: overlayFS,
},
);

assertBundles(b, [
{
assets: ['index.html'],
},
{
assets: ['style.css', 'common.css'],
},
{
assets: ['index.js', 'bundle-manifest.js', 'esm-js-loader.js'],
},
{
assets: ['async.js'],
},
]);
});
});
30 changes: 14 additions & 16 deletions packages/core/integration-tests/test/css-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -619,29 +619,19 @@ describe('css modules', () => {
},
{
type: 'js',
assets: [
'page1.js',
'index.module.css',
'a.module.css',
'b.module.css',
],
assets: ['page1.js'],
},
{
type: 'js',
assets: [
'page2.js',
'index.module.css',
'a.module.css',
'b.module.css',
],
assets: ['page2.js'],
},
{
type: 'css',
assets: ['a.module.css', 'b.module.css'],
assets: ['a.module.css', 'b.module.css', 'index.module.css'],
},
{
type: 'css',
assets: ['index.module.css'],
type: 'js',
assets: ['a.module.css', 'b.module.css', 'index.module.css'],
},
Copy link
Member Author

Choose a reason for hiding this comment

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

these changes are due to turning on "minBundleSize": 0

]);
});
Expand Down Expand Up @@ -745,7 +735,7 @@ describe('css modules', () => {
assert(res[0][1].includes('container') && res[0][1].includes('expand'));
});

it('should allow css modules to be shared between targets', async function () {
it('should duplicate css modules between targets', async function () {
let b = await bundle([
path.join(__dirname, '/integration/css-module-self-references/a'),
path.join(__dirname, '/integration/css-module-self-references/b'),
Expand All @@ -760,6 +750,14 @@ describe('css modules', () => {
name: 'main.css',
assets: ['bar.module.css'],
},
{
name: 'module.css',
assets: ['bar.module.css'],
},
{
name: 'module.css',
assets: ['bar.module.css'],
},
{
name: 'main.js',
assets: ['index.js', 'bar.module.css'],
Expand Down
9 changes: 1 addition & 8 deletions packages/core/integration-tests/test/css.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,19 +77,12 @@ describe('css', () => {
{
name: 'entry.js',
type: 'js',
assets: [
'bundle-url.js',
'cacheLoader.js',
'css-loader.js',
'entry.js',
'js-loader.js',
],
assets: ['bundle-url.js', 'cacheLoader.js', 'entry.js', 'js-loader.js'],
},
{
type: 'js',
assets: ['esmodule-helpers.js', 'index.js'],
},
{name: 'Foo.css', type: 'css', assets: ['foo.css']},
{name: 'entry.css', type: 'css', assets: ['foo.css', 'main.css']},
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this was actually a similar case to the one I originally described in the description. foo.css doesn't need to be duplicated because it's already loaded in a parallel bundle

]);
});
Expand Down
38 changes: 23 additions & 15 deletions packages/core/integration-tests/test/html.js
Original file line number Diff line number Diff line change
Expand Up @@ -2242,39 +2242,39 @@ describe('html', function () {

assertBundles(b, [
{
name: 'a.html',
type: 'html',
type: 'js',
assets: ['a.html'],
},
{
name: 'b.html',
type: 'html',
type: 'js',
assets: ['b.html'],
},
{
name: 'c.html',
type: 'html',
type: 'js',
assets: ['c.html'],
},
{
type: 'js',
assets: ['a.html', 'shared.js'],
name: 'a.html',
type: 'html',
assets: ['a.html'],
},
{
type: 'js',
assets: ['b.html', 'shared.js'],
name: 'b.html',
type: 'html',
assets: ['b.html'],
},
{
type: 'js',
assets: ['c.html', 'shared.js'],
name: 'c.html',
type: 'html',
assets: ['c.html'],
},
{
type: 'css',
assets: ['other.css'],
assets: ['other.css', 'shared.css'],
},
{
type: 'css',
assets: ['shared.css'],
type: 'js',
assets: ['shared.js'],
},
]);

Expand Down Expand Up @@ -2493,6 +2493,14 @@ describe('html', function () {
type: 'css',
assets: ['a.module.css'],
},
{
type: 'css',
assets: ['a.module.css'],
},
{
type: 'css',
assets: ['a.module.css'],
},
{
type: 'html',
assets: ['searchfield.html'],
Copy link
Member Author

Choose a reason for hiding this comment

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

above changes are also due to "minBundleSize"

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"@parcel/bundler-default": {
"minBundleSize": 0
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"@parcel/bundler-default": {
"minBundleSize": 0
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"@parcel/bundler-default": {
"minBundleSize": 0
}
}
28 changes: 24 additions & 4 deletions packages/core/integration-tests/test/monorepos.js
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,10 @@ describe('monorepos', function () {
name: 'pkg-b.cjs.css',
assets: ['index.module.css'],
},
{
name: 'pkg-b.module.css',
assets: ['index.module.css'],
},
]);

let contents = await outputFS.readFile(
Expand Down Expand Up @@ -300,7 +304,7 @@ describe('monorepos', function () {
),
'utf8',
);
assert(contents.includes('import "./pkg-b.cjs.css"'));
assert(contents.includes('import "./pkg-b.module.css"'));
});

it('should build using root targets with a glob pointing at files inside packages and cwd at project root', async function () {
Expand Down Expand Up @@ -531,6 +535,10 @@ describe('monorepos', function () {
name: 'pkg-b.cjs.css',
assets: ['index.module.css'],
},
{
name: 'pkg-b.module.css',
assets: ['index.module.css'],
},
]);

let contents = await outputFS.readFile(
Expand Down Expand Up @@ -576,7 +584,7 @@ describe('monorepos', function () {
),
'utf8',
);
assert(contents.includes('import "./pkg-b.cjs.css"'));
assert(contents.includes('import "./pkg-b.module.css"'));
});

it('should watch glob entries and build new packages that are added', async function () {
Expand Down Expand Up @@ -634,6 +642,10 @@ describe('monorepos', function () {
name: 'pkg-b.cjs.css',
assets: ['index.module.css'],
},
{
name: 'pkg-b.module.css',
assets: ['index.module.css'],
},
]);
});

Expand Down Expand Up @@ -788,6 +800,10 @@ describe('monorepos', function () {
name: 'pkg-a.cjs.css',
assets: ['index.module.css'],
},
{
name: 'pkg-a.module.css',
assets: ['index.module.css'],
},
{
name: 'pkg-b.cjs.js',
assets: ['index.js', 'index.module.css'],
Expand All @@ -800,6 +816,10 @@ describe('monorepos', function () {
name: 'pkg-b.cjs.css',
assets: ['index.module.css'],
},
{
name: 'pkg-b.module.css',
assets: ['index.module.css'],
},
]);

let contents = await outputFS.readFile(
Expand All @@ -820,7 +840,7 @@ describe('monorepos', function () {
'utf8',
);
assert(contents.includes('export {'));
assert(contents.includes('import "./pkg-a.cjs.css"'));
assert(contents.includes('import "./pkg-a.module.css"'));

contents = await outputFS.readFile(
path.join(
Expand Down Expand Up @@ -856,7 +876,7 @@ describe('monorepos', function () {
),
'utf8',
);
assert(contents.includes('import "./pkg-b.cjs.css"'));
assert(contents.includes('import "./pkg-b.module.css"'));
});

Copy link
Member Author

Choose a reason for hiding this comment

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

changes in this file are due to css bundles no longer being reused between different targets, so there is now both a CJS and ESM version

it('should search for .parcelrc at cwd in monorepos', async () => {
Expand Down