fix(plugin-legacy): improve deterministic polyfills discovery #16566

shankerwangmiao · 2024-04-30T16:59:14Z

Description

The polyfill bundle generated by the legacy plugin is non deterministic, because the order of calling renderChunk() affects the input of the finally generated polyfill bundle.

This PR solves this issue by sorting the polyfills in need discovered by babel. The polyfills added directly from the plugin configuration is kept ordered the first, same as before.

Previously, #13672 (comment) mentioned this issue.

stackblitz · 2024-04-30T16:59:16Z

Run & review this pull request in StackBlitz Codeflow.

bluwy

Could we sort (and convert to array for) the polyfills before these two spots instead?

vite/packages/plugin-legacy/src/index.ts

Lines 269 to 273 in 2322657

    
           isDebug && 
        
             console.log( 
        
               `[@vitejs/plugin-legacy] modern polyfills:`, 
        
               modernPolyfills, 
        
             )

vite/packages/plugin-legacy/src/index.ts

Lines 306 to 310 in 2322657

    
           isDebug && 
        
             console.log( 
        
               `[@vitejs/plugin-legacy] legacy polyfills:`, 
        
               legacyPolyfills, 
        
             )

We can then pass the sorted array to buildPolyfillChunk() below them and it should be deterministic?

shankerwangmiao · 2024-05-01T13:46:22Z

Could we sort (and convert to array for) the polyfills before these two spots instead?

vite/packages/plugin-legacy/src/index.ts

Lines 269 to 273 in 2322657

isDebug &&

console.log(

`[@vitejs/plugin-legacy] modern polyfills:`,

modernPolyfills,

)

vite/packages/plugin-legacy/src/index.ts

Lines 306 to 310 in 2322657

isDebug &&

console.log(

`[@vitejs/plugin-legacy] legacy polyfills:`,

legacyPolyfills,

)

We can then pass the sorted array to buildPolyfillChunk() below them and it should be deterministic?

Hi, thanks for your review. It could be simpler to directly sort the array here. However, I think the ordering of polyfill groups should be kept as before. Originally, the corejs polyfills specified in options.{modern,legacy}Polyfills comes the first, followed by those specified in additional{Modern,Legacy}Polyfills, and those discovered by babel. If we directly sort the combined polyfills list, this ordering will be broken, and some projects may rely on this to work properly. This proposed change keeps this ordering, and only sort the polyfills generated by babel.

shankerwangmiao · 2024-05-01T13:55:38Z

Also some issues with the CI. I found that the CI sometimes fails on tests not related with legacy plugin.

See:

They should be running on the same changes, but I have no idea why some of the tests failed

bluwy · 2024-05-02T04:17:11Z

Yeah I'd ignore the CI fails that aren't related to plugin-legacy. It's flaky lately.

If we directly sort the combined polyfills list, this ordering will be broken, and some projects may rely on this to work properly.

I did some digging and found babel/babel-polyfills#192. So it seems like some order matters and presumably babel-preset-env will respect that when it returns. But in your PR, you're sorting the list too, which is incorrect?

In which case, the current behaviour is safer wrt ordering. Otherwise it's better to sort the entire array based based on core-js-compat/modules.json instead?

shankerwangmiao · 2024-05-02T04:51:24Z

Ah, thanks for pointing it out. I'll see how to make the polyfill list stable and following the correct ordering requirement.

shankerwangmiao · 2024-05-02T18:27:52Z

I did some digging and found babel/babel-polyfills#192. So it seems like some order matters and presumably babel-preset-env will respect that when it returns. But in your PR, you're sorting the list too, which is incorrect?

In which case, the current behaviour is safer wrt ordering. Otherwise it's better to sort the entire array based based on core-js-compat/modules.json instead?

Hi, I believe sorting according to core-js-compat/modules.json is definitely correct. However, by doing that, we are skipping the abstract layer provided by babel/preset-env, so I don't think it will be a clean solution.

My idea is to record all the generated polyfills and their ordering in each chunk and generate a combined list of polyfills by doing a topological sort, and generating a lexicographically minimum one to make the result stable.

However, by implementing it, as you can see from the CI result, I discovered that the order of polyfills given by babel is not strictly following the order in core-js-compat/modules.json. For example, in playground/legacy, the polyfills in the legacy chunk assets/chunk-main-legacy.!~{003}~.js is

core-js/modules/es.symbol.js
core-js/modules/es.symbol.description.js
core-js/modules/es.symbol.async-iterator.js
core-js/modules/es.symbol.iterator.js
core-js/modules/es.symbol.to-string-tag.js
core-js/modules/es.error.cause.js
core-js/modules/es.array.concat.js
core-js/modules/es.array.from.js
core-js/modules/es.array.iterator.js
core-js/modules/es.array.join.js
core-js/modules/es.array.map.js
core-js/modules/es.array.push.js
core-js/modules/es.function.name.js
core-js/modules/es.json.stringify.js
core-js/modules/es.json.to-string-tag.js
core-js/modules/es.map.js
core-js/modules/es.math.to-string-tag.js
core-js/modules/es.object.freeze.js
core-js/modules/es.object.get-prototype-of.js
core-js/modules/es.object.keys.js
core-js/modules/es.array.slice.js
core-js/modules/es.object.to-string.js
core-js/modules/es.promise.js
core-js/modules/es.set.js
core-js/modules/es.string.ends-with.js
core-js/modules/es.regexp.exec.js
core-js/modules/es.regexp.test.js
core-js/modules/es.regexp.to-string.js
core-js/modules/es.string.iterator.js
core-js/modules/es.string.raw.js
core-js/modules/esnext.iterator.constructor.js
core-js/modules/esnext.iterator.for-each.js
core-js/modules/esnext.iterator.map.js
core-js/modules/esnext.set.difference.v2.js
core-js/modules/esnext.set.intersection.v2.js
core-js/modules/esnext.set.is-disjoint-from.v2.js
core-js/modules/esnext.set.is-subset-of.v2.js
core-js/modules/esnext.set.is-superset-of.v2.js
core-js/modules/esnext.set.symmetric-difference.v2.js
core-js/modules/esnext.set.union.v2.js
core-js/modules/web.dom-collections.for-each.js
core-js/modules/web.dom-collections.iterator.js
core-js/modules/web.dom-exception.constructor.js
core-js/modules/web.dom-exception.stack.js
core-js/modules/web.dom-exception.to-string-tag.js
core-js/modules/web.structured-clone.js
core-js/modules/web.url.js
core-js/modules/web.url.to-json.js
core-js/modules/web.url-search-params.js
core-js/modules/web.url-search-params.delete.js
core-js/modules/web.url-search-params.has.js
core-js/modules/web.url-search-params.size.js

We can see core-js/modules/es.array.slice.js is ordered after core-js/modules/es.object.keys.js which is opposite to the ordering in core-js-compat/modules.json.

I wonder whether this is a bug in babel or intended.

shankerwangmiao · 2024-05-02T18:58:53Z

This issue is addressed by adding a circle breaking mechanism.

yoyo837 · 2024-05-07T01:20:20Z

Thanks for the great job. Really looking forward to this fix.

bluwy · 2024-05-08T05:28:44Z

Thanks for investigating this @shankerwangmiao. Seems like babel-preset-env might have a bug with the ordering but I can't find out why.

From the current implementation, I don't understand why we need a complex sorting algorithm to sort it though. Why not do a single final sort based on core-js-compat/modules.json? The sort function should be something like this.

shankerwangmiao · 2024-05-08T06:04:10Z

From the current implementation, I don't understand why we need a complex sorting algorithm to sort it though. Why not do a single final sort based on core-js-compat/modules.json? The sort function should be something like this.

Because there is no insurance that someday babel won't add other polyfill dependency from other packages, I believe not directly relying on the core-js-compat/modules.json, but relying on solely the output of babel would be a better approach.

bluwy · 2024-05-08T06:25:49Z

If babel adds another polyfill, wouldn't that still be fine? It shouldn't be having a specific order if it's not core-js, but if it needs a specific order, we can add it later too.

Perhaps what I don't understand too is that, if babel-preset-env outputs imports that aren't in the correct order, how does a sorting algorithm fix into a correct order, and especially would always respect core-js-compat/modules.json without reading it?

I can see that it's in the correct order now locally, but will it always be in the correct order if, let's say, there's only one chunk which babel outputs in an incorrect order?

shankerwangmiao · 2024-05-08T20:44:11Z

If babel adds another polyfill, wouldn't that still be fine? It shouldn't be having a specific order if it's not core-js, but if it needs a specific order, we can add it later too.

Perhaps what I don't understand too is that, if babel-preset-env outputs imports that aren't in the correct order, how does a sorting algorithm fix into a correct order, and especially would always respect core-js-compat/modules.json without reading it?

I can see that it's in the correct order now locally, but will it always be in the correct order if, let's say, there's only one chunk which babel outputs in an incorrect order?

The basic assumption is that the ordering given by babel is (or should be) always right, no matter whether the polyfills are from core-js or other sources. We utilize this information rather than the information from core-js-compat/modules.json to keep all the polyfills in the correct order. The "correct order" means that if a polyfill A comes before B in a chunk, A will come before B in the final output; if a polyfill A and a polyfill B never appear in the same chunk, there will be no ordering constrain between them.

bluwy · 2024-05-09T04:15:22Z

Even if babel is correctly outputting in the right order today, there's still the problem where additionalModernPolyfills or additionalLegacyPolyfills get merged and there are not in the right order. Having in the incorrect order is very unlikely to be intended, so we should also sort and fix it up.

I get that fixing the ordering issue wasn't the initial goal of this PR, it was to only make it deterministic. But knowing now that the bug exist, and the way to solve this is to sort based on core-js-compat/modules.json, we can do that directly to kill two birds with one stone? Otherwise we have to followup with another PR that does that.

shankerwangmiao · 2024-05-09T09:48:40Z

You are right about this. My original point is to only keep the output deterministic, not to solve the ordering issue.

If we sort according to core-js-compat/modules.json, what if the user choose to include polyfills from other than those provided by core-js in additional*Polyfills? Also, currently, additional*Polyfills comes before the polyfills imported by babel, what if the user relies on the ordering?

bluwy · 2024-05-09T10:44:18Z

What I had in mind is that, if there's any polyfills that aren't within core-js-compat/modules.json, then we keep the ordering as-is. However, it is more complex I think 🤔 For example:

// es.array.slice must be before es.object.keys

// Case 1:
const additional =  ['core-js/modules/es.array.slice.js', 'something-else.js']
const babel = ['core-js/modules/es.object.keys.js']
const result = ['core-js/modules/es.array.slice.js', 'something-else.js', 'core-js/modules/es.object.keys.js']

// Case 2:
const additional =  ['core-js/modules/es.object.keys.js', 'something-else.js']
const babel = ['core-js/modules/es.array.slice.js']
const result = ['core-js/modules/es.array.slice.js', 'core-js/modules/es.object.keys.js', 'something-else.js']
// Move es.array.slice before es.object.keys

I'm not sure if a .sort algorithm would be enough to do the trick, but I think keeping the basic idea of moving things to the front should make sure non-corejs modules are not simply moved entirely to last. In other words, non-corejs modules if it has another polyfill before it, there shouldn't be something injected between them. I might need to think about this more.

shankerwangmiao · 2024-05-10T06:41:43Z

I guess a simple sort is not enough. Maybe we need topology sorting. However, I have no idea on how to deal with contradictory ordering constrains.

Huodoo · 2024-05-14T06:41:46Z

I guess a simple sort is not enough. Maybe we need topology sorting. However, I have no idea on how to deal with contradictory ordering constrains.

Maybe a sorting algorithm is enough. This will just sort the polyfill import statement

bluwy · 2024-05-14T08:37:11Z

I had a few ideas recently to solve the ordering issue, but all are quite complex. @shankerwangmiao I'm happy to solve the deterministic issue for now, but I think we could simplify the implementation.

The forth argument of renderChunk has meta.chunks. This object is properly ordered. On the first renderChunk call, we can create a map like this:

chunkFileNameToPolyfills = new Map<string, { modern: Set<string>, legacy: Set<string> }>();
for (const fileName in meta.chunks) {
  chunkFileNameToPolyfills.set(fileName,  { modern: new Set(), legacy: new Set() }
}

And then later on the code in renderChunk will always only populate the sets in chunkFileNameToPolyfills. And finally in generateBundle, we can iterate chunkFileNameToPolyfills and combine it all back into the main modernPolyfills and legacyPolyfills. What do you think?

shankerwangmiao · 2024-05-14T10:09:38Z

I followed your suggestion, but with a simpler solution. In renderChunk(), chunkName and polyfills are recorded in an array. In generateBundle(), we simply sort it according the chunkName, and combine the polyfills together.

The change made to the original process is simple: only the order of polyfill groups generated by chunks and pushed to {legacy,modern}Polyfills is changed.

Huodoo · 2024-05-14T11:05:26Z

I had a few ideas recently to solve the ordering issue, but all are quite complex. @shankerwangmiao I'm happy to solve the deterministic issue for now, but I think we could simplify the implementation.

The forth argument of renderChunk has meta.chunks. This object is properly ordered. On the first renderChunk call, we can create a map like this:
chunkFileNameToPolyfills = new Map<string, { modern: Set<string>, legacy: Set<string> }>();
for (const fileName in meta.chunks) {
  chunkFileNameToPolyfills.set(fileName,  { modern: new Set(), legacy: new Set() }
}
And then later on the code in renderChunk will always only populate the sets in chunkFileNameToPolyfills. And finally in generateBundle, we can iterate chunkFileNameToPolyfills and combine it all back into the main modernPolyfills and legacyPolyfills. What do you think?

modernPolyfills and legacyPolyfills is just used for generate a file like

import "core-js/modules/es.regexp.exec.js";import "core-js/modules/es.regexp.test.js";import "core-js/modules/es.regexp.to-string.js";import "core-js/modules/es.string.match.js";import "core-js/modules/es.string.replace.js";

#16668

bluwy · 2024-05-14T11:40:51Z

@Huodoo please read the above discussion, particularly #16566 (comment). The import order matters.

packages/plugin-legacy/src/index.ts

…rting the polyfills discovered by babel

shankerwangmiao · 2024-05-21T23:42:58Z

ping @bluwy

bluwy · 2024-05-28T06:50:54Z

I'm not sure if I missed something, but I pushed a refactor implementing #16566 (comment) which I felt was simpler and has less memory-copying with the data structures. I also confirmed that the discovered modern and legacy polyfills were still the same order.

shankerwangmiao · 2024-05-28T22:52:23Z

I'm not sure if I missed something, but I pushed a refactor implementing #16566 (comment) which I felt was simpler and has less memory-copying with the data structures. I also confirmed that the discovered modern and legacy polyfills were still the same order.

I also implemented that in 37c344f, before I pinged you. I think your implementation is better. I've tested the refactored version and the output remains the same.

I wonder if we are ready to have it merged.

sapphi-red

IIUC this implementation is based on the assumption that:

the injection order we need to follow for core-js is determined by a dependency graph rather than a list (i.e. modules.json is a list, but actually it includes some orders that doesn't need to be followed)
babel injects all the ancestor polyfills (polyfills which is required by the polyfills injected for the source code)
babel injects the polyfills in the correct order

I guess all the assumptions are met and I think this would work.

bluwy · 2024-05-29T08:54:53Z

@sapphi-red I think at the moment we had discovered that Babel doesn't necessarily return the polyfills in the right order, and there's definitely a chance the order is incorrect due to different chunks that can contain different code. But in my comment I figured that it's easier to tackle this deterministic issue first and figure out the ordering later as it can be tricky to sort with non corejs modules (sorry for the long discussion here).

sapphi-red · 2024-05-29T11:15:58Z

Ah, I see. Then, I don't think this will break things so I think it's fine.

bluwy reviewed May 1, 2024

View reviewed changes

bluwy added plugin: legacy p2-nice-to-have Not breaking anything but nice to have (priority) labels May 1, 2024

bluwy mentioned this pull request May 1, 2024

feat(plugin-legacy): support additionalModernPolyfills #16514

Merged

ModyQyW mentioned this pull request May 6, 2024

feat: align with @vitejs/[email protected] CyanSalt/vite-plugin-legacy-swc#11

Merged

bluwy mentioned this pull request May 14, 2024

fix(legacy): unexpected hash change #16668

Closed

bluwy reviewed May 14, 2024

View reviewed changes

packages/plugin-legacy/src/index.ts Outdated Show resolved Hide resolved

fix(legacy): improve deterministic build of the polyfill bundle by so…

37c344f

…rting the polyfills discovered by babel

bluwy added 2 commits May 28, 2024 14:48

chore: refactor

0686724

chore: fix typo

9d936b9

sapphi-red approved these changes May 29, 2024

View reviewed changes

bluwy changed the title ~~fix(legacy): improve deterministic build of the polyfill bundle by sorting the polyfills discovered by babel~~ fix(plugin-legacy): improve deterministic polyfills discovery May 29, 2024

bluwy merged commit 48edfcd into vitejs:main May 29, 2024
12 checks passed

bluwy mentioned this pull request May 29, 2024

fix(plugin-legacy): group discovered polyfills by output #17347

Merged

yoyo837 mentioned this pull request Jun 17, 2024

Non-deterministic build #13672

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(plugin-legacy): improve deterministic polyfills discovery #16566

fix(plugin-legacy): improve deterministic polyfills discovery #16566

shankerwangmiao commented Apr 30, 2024

stackblitz bot commented Apr 30, 2024

bluwy left a comment

shankerwangmiao commented May 1, 2024

shankerwangmiao commented May 1, 2024

bluwy commented May 2, 2024

shankerwangmiao commented May 2, 2024

shankerwangmiao commented May 2, 2024

shankerwangmiao commented May 2, 2024

yoyo837 commented May 7, 2024

bluwy commented May 8, 2024

shankerwangmiao commented May 8, 2024

bluwy commented May 8, 2024

shankerwangmiao commented May 8, 2024

bluwy commented May 9, 2024

shankerwangmiao commented May 9, 2024

bluwy commented May 9, 2024

shankerwangmiao commented May 10, 2024

Huodoo commented May 14, 2024

bluwy commented May 14, 2024

shankerwangmiao commented May 14, 2024 •

edited

Loading

Huodoo commented May 14, 2024

bluwy commented May 14, 2024

shankerwangmiao commented May 21, 2024

bluwy commented May 28, 2024

shankerwangmiao commented May 28, 2024

sapphi-red left a comment

bluwy commented May 29, 2024

sapphi-red commented May 29, 2024

	isDebug &&
	console.log(
	`[@vitejs/plugin-legacy] modern polyfills:`,
	modernPolyfills,
	)

	isDebug &&
	console.log(
	`[@vitejs/plugin-legacy] legacy polyfills:`,
	legacyPolyfills,
	)

fix(plugin-legacy): improve deterministic polyfills discovery #16566

fix(plugin-legacy): improve deterministic polyfills discovery #16566

Conversation

shankerwangmiao commented Apr 30, 2024

Description

stackblitz bot commented Apr 30, 2024

bluwy left a comment

Choose a reason for hiding this comment

shankerwangmiao commented May 1, 2024

shankerwangmiao commented May 1, 2024

bluwy commented May 2, 2024

shankerwangmiao commented May 2, 2024

shankerwangmiao commented May 2, 2024

shankerwangmiao commented May 2, 2024

yoyo837 commented May 7, 2024

bluwy commented May 8, 2024

shankerwangmiao commented May 8, 2024

bluwy commented May 8, 2024

shankerwangmiao commented May 8, 2024

bluwy commented May 9, 2024

shankerwangmiao commented May 9, 2024

bluwy commented May 9, 2024

shankerwangmiao commented May 10, 2024

Huodoo commented May 14, 2024

bluwy commented May 14, 2024

shankerwangmiao commented May 14, 2024 • edited Loading

Huodoo commented May 14, 2024

bluwy commented May 14, 2024

shankerwangmiao commented May 21, 2024

bluwy commented May 28, 2024

shankerwangmiao commented May 28, 2024

sapphi-red left a comment

Choose a reason for hiding this comment

bluwy commented May 29, 2024

sapphi-red commented May 29, 2024

shankerwangmiao commented May 14, 2024 •

edited

Loading