fix(legacy): ensure isLegacyBundle checks all entry chunks for legacy suffix

[sapenlei]

fix(plugin-legacy): ensure isLegacyBundle checks all entry chunks for legacy suffix

The previous implementation of isLegacyBundle found the first entry chunk and then checked if it had a '-legacy' suffix. This could lead to incorrect detection in multi-entry scenarios.

For example:
If a bundle contains:

1. An entry chunk with filename "other.js" (without "-legacy")
2. And another entry chunk with filename "main-legacy.js" (with "-legacy")

The original implementation would return false because it would find "other.js" first and check if it includes "-legacy", which it doesn't.

With this fix, the function directly searches for any entry chunk that has the "-legacy" suffix, correctly identifying bundles with any legacy entry chunks.

This ensures proper asset handling in the generateBundle hook, especially for multi-entry builds where some entries might be legacy and others modern.

[sapphi-red]

This could lead to incorrect detection in multi-entry scenarios.

For example: If a bundle contains:

1. An entry chunk with filename "other.js" (without "-legacy")
2. And another entry chunk with filename "main-legacy.js" (with "-legacy")

How does this happen? plugin-legacy sets output.entryChunkFileNames to include -legacy. So in this case, that bundle is actually not a legacy bundle.
Maybe the fix should be changing .find into .all?

This ensures proper asset handling in the generateBundle hook, especially for multi-entry builds where some entries might be legacy and others modern.

Would you elaborate on this sentence? generateBundle hook is called for each value in the output option and the modern bundle and the legacy bundle are separate values. IIUC legacy chunks won't be included in bundle parameter of generateBundle hook when generating modern chunks, and vice versa.

[sapenlei]

How does this happen? plugin-legacy sets output.entryChunkFileNames to include -legacy. So in this case, that bundle is actually not a legacy bundle.

This ensures proper asset handling in the generateBundle hook, especially for multi-entry builds where some entries might be legacy and others modern.

Would you elaborate on this sentence?

https://bolt.new/~/vitejs-vite-4wh9epy3
There are two entry chunks in this Vite project, one is remoteEntry.js (generated by Vite Module Federation) and the other is main-legacy.js.

[sapphi-red]

Does that work? It seems the remoteEntry.js output by the legacy build is overridden by the modern build.

[sapenlei]

How does this happen? plugin-legacy sets output.entryChunkFileNames to include -legacy. So in this case, that bundle is actually not a legacy bundle. Maybe the fix should be changing .find into .all?

Many tests in legacy.spec.ts are failing. The original implementation considered a bundle to be a legacy bundle if it had one entry chunk marked as legacy.

The original implementation has a problem: if there are two entry chunks, where the first is not legacy but the second one is legacy, the function should return true instead of false.

So I think this change is correct.

[sapphi-red]

I was thinking of the following code when I said "Maybe the fix should be changing .find into .all?":

Object.values(bundle).every((output) =>
  output.type === 'chunk' && output.isEntry ? output.fileName.includes('-legacy') : true,
)

This is because I expect all entry chunks that are generated in the legacy bundle to have -legacy.

So I think this change is correct.

I'm not saying your change is incorrect. I'm trying to understand why the change is needed.
I didn't understand why the example you showed (https://bolt.new/~/vitejs-vite-4wh9epy3) works. The remoteEntry.js output for the legacy bundle is overridden by the modern bundle, so legacy browsers won't be able to execute the remoteEntry.js chunk and thus I thought it won't work.

[sapenlei]

I was thinking of the following code when I said "Maybe the fix should be changing .find into .all?":

Object.values(bundle).every((output) =>
  output.type === 'chunk' && output.isEntry ? output.fileName.includes('-legacy') : true,
)

This is because I expect all entry chunks that are generated in the legacy bundle to have -legacy.

Okay, So next, I'm just going to fix these failed tests?

[sapphi-red]

I didn't understand why the example you showed (https://bolt.new/~/vitejs-vite-4wh9epy3) works. The remoteEntry.js output for the legacy bundle is overridden by the modern bundle, so legacy browsers won't be able to execute the remoteEntry.js chunk and thus I thought it won't work.

Would you explain this first? I'm not quite sure what this PR is fixing.

[sapenlei]

I didn't understand why the example you showed (https://bolt.new/~/vitejs-vite-4wh9epy3) works. The remoteEntry.js output for the legacy bundle is overridden by the modern bundle, so legacy browsers won't be able to execute the remoteEntry.js chunk and thus I thought it won't work.

Would you explain this first? I'm not quite sure what this PR is fixing.

Before the modification, the polyfill wasn't generated and systemJs wasn't included, which resulted in the error "system is not defined". After the modification, the polyfill is included, so the "system is not defined" error no longer appears.

[sapphi-red]

My understanding of your reproduction is:

• your application is a remote application of module federation
• to use a remote application, you load the remoteEntry.js

But remoteEntry.js output contains modern syntax and would not work in legacy browsers. How do you make this work?