-
-
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
Experimental bundler integration #8180
Conversation
* Traverse each bundle instead of iterating each outbound node * Add edge between root and bundle
This reverts commit 73ad828.
1ed5a4e
to
0c3a653
Compare
…/parcel-bundler/parcel into experimental-bundler-integration
…perimental-bundler-integration
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.
Looks good overall. Couple questions.
Would be great to have a post with diagrams, a slide deck, or something a bit more visual to go through all the steps. The code comments are helpful, but I struggled without visual aids and examples of scenarios we covered. Probably a good idea to do before we forget, but certainly not a blocker for this PR.
); | ||
assert.equal(html.match(/<script/g)?.length, 7); | ||
|
||
assertBundles(b.bundleGraph, [ |
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.
Why did these cache tests change?
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.
We changed the cache tests because they didn't seem to test the config properly, now the test for adding a config
and removing
determine if minBundleSize was upheld, for bundles with a large asset.
The previous test was testing script tags within a bundle, so it didn't seem like it was just testing bundler config.
if ( | ||
entries.has(a) || | ||
!a.isBundleSplittable || | ||
getBundleFromBundleRoot(a).needsStableName || |
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.
Are we sure this is necessary and not already covered by entries? I'm not sure if we want to conflate naming with bundling behavior?
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.
I think we took this logic directly from the default bundler (see this link to the code)
Yeah I'm kind of confused about this myself, I know non-entry bundles can be non-splittable and needStableName so that's why that is there, but I'm not sure if only bundlingBehavior can cover that. NeedsStableName
is determined by bundlingBehavior but not equal.
As for isBundleSplittable
, I believe that comes from the Asset itself, so I'm not sure how thats determined exactly but I think it's set before bundling occurs
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.
Oh hmm I don't remember why we never changed this. Ok, well we can come back to it.
The flow comments in 15085a2 should rather be solved with |
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.
lgtm!
↪️ Pull Request
This PR handles
✔️ PR Todo