fix: handle new virtual legacy-polyfill name #157
Conversation
The entry of the legacy-polyfill name has changed to start with a \u0000 character to mark it as a virtual entry in vite 2.7. This commit ensures that this new name is handled correctly. See: #156
|
Hi Stef, thanks for providing detailed information, a potential fix, and updating the tests 😃 |
| @@ -147,9 +147,11 @@ def resolve_references(manifest) | |||
|
|
|||
| # Internal: Resolves the manifest entry name for the specified resource. | |||
| def resolve_entry_name(name, type: nil) | |||
| name = with_file_extension(name.to_s, type) | |||
| unless name.include?('legacy-polyfills') | |||
There was a problem hiding this comment.
Perhaps we could take this opportunity to generalize the check:
unless name.include?("\u0000")There was a problem hiding this comment.
I think that a part of your comment didn't come through properly but I think you want to check on the null character itself? I think that could work as well. Another solution is to only do this file extension check if a type is explicitly passed.
But since I wasn't too sure about other effect, I took the minimal change to make it happen. Last time I made a PR, I broke the workflow of other users 🙈
And maybe also something to take in consideration: If the users using this gem did NOT upgrade vite itself yet. Then this null character is not yet in the manifest.
There was a problem hiding this comment.
Sorry, on my phone, hence the conciseness 😅
What I suggest is changing the condition to check for the presence of the null char in the provided name, skipping the file extension check (since adding the extension is not relevant when looking up virtual entries).
It's true that it wouldn't be compatible with Vite 2.6, but we could bump the recommended Vite version to 2.7, which is checked on startup, and would be caught during development or in the CI, where the error would suggest users to run bundle exec vite upgrade.
|
Although I'd like to remove the explicit check for legacy polyfills, I can always do that in later versions, in a major or minor release. Thanks again for the detailed report and for providing a fix ❤️ I'll cut a patch release later this week. |
|
Yea agree that it would be cleaner to have the check removed. But I also feel that this part is a bit under-tested. I also had the idea of adding the vite version to the test matrix so this could have been catched but that was a bit too much for now |
Description 📖
This pull request ensures that the new polyfill name with the
\u0000character is handled as expected.Background 📜
This was happening because the entry of the legacy-polyfill name has changed to start with a
\u0000character to mark it as a virtual entry in vite 2.7.The Fix 🔨
By changing the lookup to not try to parse the entry name, the
\u0000character is ignored.Closes #156
Screenshots 📷