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

ExperimentalBundler: merge bundleBehavior for isolated dependencies #8052

Closed

Conversation

wbinnssmith
Copy link
Contributor

@wbinnssmith wbinnssmith commented May 3, 2022

This:

  • Adds a nullable property mainEntryAsset to bundle objects in the ExperimentalBundler for ease of reference
  • Combines bundles down to the "lowest common denominator" by overriding bundleBehavior and needsStableName based on other incoming dependencies. This is a breaking change from the current bundler, trading potential asset deduplication for bundle deduplication.

One case we'll need to handle before landing:
* [ ] Marking a bundle as existing both as an inline bundle and a non-inline bundle. Currently we never write inline bundles to disk. In this case we'll need to. Let's defer this, as it seems like different asset bundleBehavior creates different assets, and as far as I can tell, it's not possible to have a non-inline asset and an inline dependency.

Test Plan: Adjusted existing test(s) to use the reduced bundle count

@wbinnssmith wbinnssmith marked this pull request as draft May 3, 2022 23:32
@wbinnssmith wbinnssmith force-pushed the wbinnssmith/multiple-bundles-same-bundle-root branch from e031471 to 672a414 Compare May 4, 2022 00:19
@parcel-benchmark
Copy link

parcel-benchmark commented May 4, 2022

Benchmark Results

Kitchen Sink 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

React HackerNews ✅

Timings

Description Time Difference
Cold 9.15s +19.00ms
Cached 441.00ms +4.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 1.58m -964.00ms
Cached 2.58s +4.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle Size Difference Time Difference
dist/esm.6190c9ac.js 61.54kb +0.00b 39.21s -33.05s 🚀
dist/index.html 240.00b +0.00b 35.47s -36.82s 🚀

Three.js ✅

Timings

Description Time Difference
Cold 6.58s +34.00ms
Cached 268.00ms -10.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@wbinnssmith wbinnssmith changed the title ExperimentalBundler: merge bundleBehavior and needsStableName ExperimentalBundler: merge bundleBehavior for isolated dependencies May 4, 2022
@wbinnssmith wbinnssmith marked this pull request as ready for review May 4, 2022 00:21
@wbinnssmith wbinnssmith force-pushed the wbinnssmith/multiple-bundles-same-bundle-root branch from 672a414 to 95b8588 Compare May 4, 2022 18:29
@devongovett
Copy link
Member

different asset bundleBehavior creates different assets

I think asset.bundleBehavior but not dependency.bundleBehavior?

let id =
opts.id ||
hashString(
(opts.sourceAssetId ?? '') +
opts.specifier +
opts.env.id +
(opts.target ? JSON.stringify(opts.target) : '') +
(opts.pipeline ?? '') +
opts.specifierType +
(opts.priority ?? 'sync'),

@devongovett devongovett self-assigned this May 9, 2022
@wbinnssmith wbinnssmith changed the base branch from more-experimental-bundler-fixes to v2 May 12, 2022 23:02
@wbinnssmith wbinnssmith force-pushed the wbinnssmith/multiple-bundles-same-bundle-root branch from 95b8588 to db0550b Compare May 12, 2022 23:03
@101arrowz
Copy link
Member

This is probably a blocker for #8000; probably best to decide how exactly duplicate assets with different bundleBehavior should be bundled before deciding whether to merge generated dependencies or not.

@devongovett
Copy link
Member

Is this still needed @wbinnssmith @AGawrys?

@devongovett
Copy link
Member

This was merged as part of another PR.

@devongovett devongovett deleted the wbinnssmith/multiple-bundles-same-bundle-root branch October 24, 2022 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants