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 duplicate asset references #9109

Merged
merged 4 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
5 changes: 5 additions & 0 deletions packages/core/core/src/BundleGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -1122,6 +1122,11 @@ export default class BundleGraph {
}

isAssetReferenced(bundle: Bundle, asset: Asset): boolean {
// If the asset is available in multiple bundles, it's referenced.
if (this.getBundlesWithAsset(asset).length > 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Just because an asset is in multiple bundles doesn't mean it is referenced between multiple bundles though right? The references could be totally self contained within that bundle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does Parcel really know how all assets are going to be loaded though? We have scenrios where we add bundles to HTML dynamically and Parcel isn't aware that the two are linked. I know this isn't ideal, but I feel like we should be resilient to these kinds of issues.

Also check out the test I added, that was failing before I made this fix and that scenario should definitely work IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Right yeah I was more wondering about it because the of the name of the function like Niklas mentioned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I hear that. I can move it the fix, however I feel like that leaves an issue in isAssetReferenced. Shouldn't it return true for the scenario I outlined in the test?

Copy link
Member

Choose a reason for hiding this comment

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

depends on the definition of "referenced" I guess. If we mean "this exact instance of an asset is referenced from another bundle", then a duplicated asset wouldn't count. But if we want duplicated assets to behave at runtime like they aren't duplicated, then it would make sense.

return true;
}

let assetNodeId = nullthrows(this._graph.getNodeIdByContentKey(asset.id));

if (
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { getValue, add } from './shared';

add();

output = import('./two').then(() => getValue());
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
let counter = 0;

export function getValue() {
return counter;
}

export function add() {
counter++;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { getValue, add } from './shared';

add();
20 changes: 20 additions & 0 deletions packages/core/integration-tests/test/javascript.js
Original file line number Diff line number Diff line change
Expand Up @@ -7472,5 +7472,25 @@ describe('javascript', function () {
let res = await run(b, null, {require: false});
assert.equal(res.output, 123);
});

it('duplicate assets should share module scope', async function () {
let b = await bundle(
[
path.join(
__dirname,
'/integration/scope-hoisting/es6/multi-entry-duplicates/one.js',
),
path.join(
__dirname,
'/integration/scope-hoisting/es6/multi-entry-duplicates/two.js',
),
],
options,
);

let result = await runBundle(b, b.getBundles()[0], {}, {require: false});

assert.equal(await result.output, 2);
});
}
});