-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 missing module in large app from Experimental Bundler #8303
Conversation
…et, add tests for removal based on limit and size
…under parallel request limit
Benchmark ResultsKitchen Sink ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. React HackerNews ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. AtlasKit Editor ✅
Timings
Cold Bundles
Cached Bundles
Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions, but lets get this in so we can test
invariant(reusableBundle !== 'root' && reusableBundle != null); | ||
reusableBundle.sourceBundles.add(candidateSourceBundleId); | ||
} else { | ||
// Asset is not a bundleRoot, but if its parent bundle can be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the direct parent? Is there any case where we'd have to do this recursively?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So actually I think the comment isn't the best. Essentially what we're trying to do here is:
Of the reachable assets for an asset, is there some common subset (under a bundleRoot) that may be reused.
So I guess it's not so much as a parent, as it is any bundleRoot that synchronously imports the asset in question. (which to me is kind of a parent? Just not the only parent)
So I guess the better way to phrase would be "if a bundleRoot of a bundle that this asset is in, can be reused by some other bundleRoot in the asset's reachable, reuse it"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it's important to note, this code path you're talking about was only added because we can't guarantee that we process a bundleRoot before any non-bundleRoot assets that are in it, when placing assets into bundles.
So like if we process a non-bundleRoot asset, we need to make sure we don't prematurely place it in, where it could've been reused by some other bundleRoot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diagrams for this section might be important 😄 ..working on it
* upstream/v2: (22 commits) Cross compile toolchains are built into docker image already Also fix release build Update centos node version v2.7.0 Changelog for v2.7.0 Use placeholder expression when replacing unused symbols (#8358) Lint (#8359) Add support for errorRecovery option in @parcel/transformer-css (#8352) VS Code Extension for Parcel (#8139) Add multi module compilation for elm (#8076) Bump terser from 5.7.2 to 5.14.2 (#8322) Bump node-forge from 1.2.1 to 1.3.0 (#8271) allow cjs config files on type module projects (#8253) inject script for hmr when there is only normal script in html (#8330) feat: support react refresh for @emotion/react (#8205) Update index.d.ts (#8293) remove charset from jsloader script set (#8346) Log resolved targets in verbose log level (#8254) Fix missing module in large app from Experimental Bundler (#8303) [Symbol Propagation] Non-deterministic bundle hashes (#8212) ...
This PR addresses a missing module where only shared bundles were considered when attempting to remove a shared bundle from a group.
When determining what source bundles need the shared-bundle-to-remove's assets pushed back into it. Instead of consulting the total BundleGroup bundles, we consulted only shared bundles. This structure would then always be empty, and so shared bundles were removed from groups but not placed into source bundles at all.
Major Changes
sharedToSourceBundles
(A structure that held shared bundle ids to source bundle ids)Reused Bundles
Reused bundles are bundles that are shared (i.e. they have source bundles) but also have a mainEntryAsset. These bundles have a referencing bundle (i.e. something that imports it) but are also exact subgraphs of other bundles. So, they can be shared, but they should also never be removed by parallel request limits since they have a referencing bundles (something that dynamically imported it, causing a new bundle to be created) (See future todo for plans to remove these as an optimization)
Diagrams
In this bundleGraph,
Bar.js
requiresa.js
andb.js
(and requiresFoo.js
througha.js
)Foo.js
requiresa.js
andb.js
a.js
requiresfoo.js
Thus,
foo.js
is a subgraph ofbar.js
and creating a shared bundle would be wrong since it would be an exact copy of thefoo
bundle and leave an empty shell bundle behind.✔️ PR Todo
Future TODO
sourceBundle
property from bundles in favor of reading graphShared Bundle under Reused Bundle Case Diagram