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

Experimental bundler integration #8180

Merged
merged 113 commits into from Jun 30, 2022
Merged

Experimental bundler integration #8180

merged 113 commits into from Jun 30, 2022

Conversation

AGawrys
Copy link
Contributor

@AGawrys AGawrys commented Jun 1, 2022

↪️ Pull Request

This PR handles

  • Bundling Multiple Targets (i.e. not sharing between targets)
  • Internalizing async dependencies
  • merging bundles of the same type into one bundle (bundlegroups allowing)
  • Adding edge types to the BundleRoot Graph to differentiate between parallel & async relationships

✔️ PR Todo

  • Update description and annotate changes on PR (or contributing doc)
  • Test on large app - missing module in large app -> Fix to come
  • Test on app (react spectrum)
  • add or alter test cases

gorakong and others added 30 commits February 8, 2022 18:37
* Traverse each bundle instead of iterating each outbound node
* Add edge between root and bundle
@AGawrys AGawrys force-pushed the experimental-bundler-integration branch from 1ed5a4e to 0c3a653 Compare June 9, 2022 17:25
contributorDocs/Bundler.md Outdated Show resolved Hide resolved
@AGawrys AGawrys requested a review from devongovett June 24, 2022 02:23
Copy link
Member

@devongovett devongovett left a 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.

packages/bundlers/experimental/src/ExperimentalBundler.js Outdated Show resolved Hide resolved
packages/bundlers/experimental/src/ExperimentalBundler.js Outdated Show resolved Hide resolved
);
assert.equal(html.match(/<script/g)?.length, 7);

assertBundles(b.bundleGraph, [
Copy link
Member

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?

Copy link
Contributor Author

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.

packages/bundlers/experimental/src/ExperimentalBundler.js Outdated Show resolved Hide resolved
if (
entries.has(a) ||
!a.isBundleSplittable ||
getBundleFromBundleRoot(a).needsStableName ||
Copy link
Member

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?

Copy link
Contributor Author

@AGawrys AGawrys Jun 27, 2022

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

Copy link
Member

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.

@mischnic
Copy link
Member

mischnic commented Jun 27, 2022

The flow comments in 15085a2 should rather be solved with nullthrows(...) instead. And then the one remaining $FlowFixMe[incompatible-call] before ALL_EDGE_TYPES is also not necessary because there's no Flow error.

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

lgtm!

@AGawrys AGawrys merged commit 1beacec into v2 Jun 30, 2022
@mischnic mischnic deleted the experimental-bundler-integration branch December 21, 2022 16:58
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

7 participants