-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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
fix(plugin-legacy): improve deterministic polyfills discovery #16566
fix(plugin-legacy): improve deterministic polyfills discovery #16566
Conversation
|
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.
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 |
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 |
Yeah I'd ignore the CI fails that aren't related to plugin-legacy. It's flaky lately.
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 |
Ah, thanks for pointing it out. I'll see how to make the polyfill list stable and following the correct ordering requirement. |
Hi, I believe sorting according to 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
We can see I wonder whether this is a bug in babel or intended. |
This issue is addressed by adding a circle breaking mechanism. |
Thanks for the great job. Really looking forward to this fix. |
Thanks for investigating this @shankerwangmiao. Seems like 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 |
Because there is no insurance that someday babel won't add other polyfill dependency from other packages, I believe not directly relying on the |
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 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 |
Even if babel is correctly outputting in the right order today, there's still the problem where 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 |
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 |
What I had in mind is that, if there's any polyfills that aren't within // 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 |
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 |
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 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 |
I followed your suggestion, but with a simpler solution. In The change made to the original process is simple: only the order of polyfill groups generated by chunks and pushed to |
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"; |
@Huodoo please read the above discussion, particularly #16566 (comment). The import order matters. |
…rting the polyfills discovered by babel
ping @bluwy |
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. |
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.
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.
@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). |
Ah, I see. Then, I don't think this will break things so I think it's fine. |
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.