Allow parallel type change bundles to be reused by async siblings

[devongovett]

This is a test for an "issue" (a potential optimization) in the bundler for discussion and eventual fix. As shown in the asset graph below, there's an entry HTML file, with a parallel dependency on a CSS file and a JS file. The CSS file in turn depends on another common CSS file. The JS file has an asynchronous dependency on another JS file, which also imports the common CSS file.

Currently, this results in common.css being broken out into a separate shared bundle, even though it would already be loaded due to the parallel dependency from the HTML file when async.js loads. In other words, I think it should be considered a reachable ancestor from async.js. This does not happen if all of the files are JS files, so I think the type change is throwing it off somehow. @AGawrys any ideas?

[devongovett]

I think the issue here is actually earlier. During the initial traversal, we eagerly create bundles for type changes. This will happen when we visit the dependency between async.js and common.css. However that happens before we have collected ancestry information, so we don't know that common.css will actually already be available so that type change bundle is not necessary.

[AGawrys]

Do you think this only happens in situations where the two .css files have this parent-child relationship (styles.css and common.css) ? Because if so, then if we allow the merging step to, it would've merged them in and that could solve our issue in the interim. @devongovett

[devongovett]

@AGawrys I pushed a fix which actually simplifies the whole bundling algorithm a bit as well. Basically, instead of considering type changes as bundle roots and eagerly creating bundles during the first pass (which we have to merge together later), we can delay creating type change bundles until the very end when we are placing assets into bundles. This way we already know all of the ancestry information and can produce more optimal output. If a reachable root for an asset is of a different type, we lazily create sibling bundles of that type (one per bundle group) and reuse them if they already exist (similar to shared bundles). For shared bundles we ensure that all assets are of the same type by adding the type to the unique key.

This allows us to remove the type change merging step completely, and rely on shared bundles to handle that. For the most part, the output is the same. However, type change merging previously did not respect the "minBundleSize" setting, and now that we are relying on shared bundles for that I had to update some tests to use that.

One other difference is that type change bundles could previously be shared between different targets unlike other types of bundles. Actually we created two separate bundles in the ideal graph, but they happened to have the same uniqueKey and so they were merged when updating the mutable bundle graph. They no longer have the same unique key (which is now based on the bundle group id), so this doesn't happen anymore. I think this is ok, not sure what benefit that was providing and it was different from every other type of bundle.

Let me know if I missed something that would require type changes to be handled early!

[devongovett]

these changes are due to turning on "minBundleSize": 0

[devongovett]

I think this was actually a similar case to the one I originally described in the description. foo.css doesn't need to be duplicated because it's already loaded in a parallel bundle

[devongovett]

above changes are also due to "minBundleSize"

[devongovett]

changes in this file are due to css bundles no longer being reused between different targets, so there is now both a CJS and ESM version

[devongovett]

fixes a separate bug that was exposed by these changes: resolving a dependency to an asset should try to resolve to an asset of the same type as the bundle it's in. without this, there was a crash in the JS packager that resolved an dependency to a CSS asset.

[AGawrys]

What happens in this test case:it.skip('create a new css bundle to maintain one css bundle per bundlegroup constraint' ?

See this comment for more context. It is basically a scenario where merging the type bundles causes over fetching, but not merging results in two css bundles for an entry bundle group.

[AGawrys]

Instead of considering type changes as bundle roots and eagerly creating bundles during the first pass (which we have to merge together later), we can delay creating type change bundles until the very end when we are placing assets into bundles. This way we already know all of the ancestry information and can produce more optimal output. If a reachable root for an asset is of a different type, we lazily create sibling bundles of that type (one per bundle group) and reuse them if they already exist (similar to shared bundles). For shared bundles we ensure that all assets are of the same type by adding the type to the unique key.

This makes a lot of sense to me, I always thought we should use ancestry & reachableRoots to determine type change asset placement 🤷

This allows us to remove the type change merging step completely, and rely on shared bundles to handle that. For the most part, the output is the same. However, type change merging previously did not respect the "minBundleSize" setting, and now that we are relying on shared bundles for that I had to update some tests to use that.

I guess what about naming? I think with the previous implementation we let type bundles have a mainEntryAsset, which became part of the name. Not sure if the mainEntryAsset had a function besides naming for these types of bundles though...

One other difference is that type change bundles could previously be shared between different targets unlike other types of bundles. Actually we created two separate bundles in the ideal graph, but they happened to have the same uniqueKey and so they were merged when updating the mutable bundle graph. They no longer have the same unique key (which is now based on the bundle group id), so this doesn't happen anymore. I think this is ok, not sure what benefit that was providing and it was different from every other type of bundle.

That was probably an oversight, I think not sharing between targets generally makes more sense

Only question I'm left with is about the over-fetching scenario I commented about above, but that was never handled properly anyway so I don't mind approving this. ✔️

[AGawrys]

Once this is merged, I can go back into the contrib docs I wrote for the bundler and update the steps/ test cases. LGTM 👍

[devongovett]

@AGawrys

What happens in this test case:it.skip('create a new css bundle to maintain one css bundle per bundlegroup constraint' ?

This is the output:

[
  {
    name: 'entry.js',
    type: 'js',
    assets: [ 'bundle-url.js', 'cacheLoader.js', 'entry.js', 'js-loader.js' ]
  },
  {
    name: 'Foo.4de85387.js',
    type: 'js',
    assets: [ 'esmodule-helpers.js', 'index.js' ]
  },
  {
    name: 'index-sib.361c9295.js',
    type: 'js',
    assets: [ 'index-sib.js' ]
  },
  { name: 'entry.css', type: 'css', assets: [ 'foo.css', 'main.css' ] }
]

So, foo.css is reused from the entry.css bundle rather than having a separate bundle. And index-sib.js was perhaps missing from the expected test result entirely?

Let me know what you think we should do with this one. For now I'll merge this and we can follow up if we want to update that test.