Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Legacy plugin not compatible with vite 2.7.x #156

Closed
2 tasks done
StefSchenkelaars opened this issue Dec 17, 2021 · 7 comments 路 Fixed by #157
Closed
2 tasks done

Legacy plugin not compatible with vite 2.7.x #156

StefSchenkelaars opened this issue Dec 17, 2021 · 7 comments 路 Fixed by #157
Labels
bug Something isn't working

Comments

@StefSchenkelaars
Copy link
Contributor

  • I have tried upgrading by running bundle update vite_ruby.
  • I have read the troubleshooting section before opening an issue.

Description 馃摉

In vite 2.7, the name of the legacy-polyfills entry has changed from "../../vite/legacy-polyfills" to "../../\u0000vite/legacy-polyfills". This filename has changed since this polyfill is a so called virtual file (as explained in the plugin api docs).

This however makes the ruby code crash since it can't resolve the filename (see stacktrace below).

This change is also discussed in vitejs/vite#6097 but as I dove into it I found that this NULL character is not a typo but a feature. So therefore I think the gem should be change to handle this correctly.

If this could be confirmed as a bug, I could work out a solution.

Reproduction 馃悶

  • Repo with vite > 2.7 and the legacy plugin
  • Render a page with a vite_legacy_javascript_tag or vite_legacy_typescript_tag tag

Logs 馃摐

ActionView::Template::Error (path name contains null byte):
vite_ruby (3.0.3) lib/vite_ruby/manifest.rb:176:in `extname'
vite_ruby (3.0.3) lib/vite_ruby/manifest.rb:176:in `with_file_extension'
vite_ruby (3.0.3) lib/vite_ruby/manifest.rb:150:in `resolve_entry_name'
vite_ruby (3.0.3) lib/vite_ruby/manifest.rb:83:in `lookup'
vite_ruby (3.0.3) lib/vite_ruby/manifest.rb:70:in `lookup!'
vite_ruby (3.0.3) lib/vite_ruby/manifest.rb:22:in `path_for'
vite_rails (3.0.2) lib/vite_rails/tag_helpers.rb:22:in `vite_asset_path'
vite_plugin_legacy (3.0.0) lib/vite_plugin_legacy/tag_helpers.rb:30:in `vite_legacy_polyfill_tag'
vite_plugin_legacy (3.0.0) lib/vite_plugin_legacy/tag_helpers.rb:15:in `vite_legacy_javascript_tag'
vite_plugin_legacy (3.0.0) lib/vite_plugin_legacy/tag_helpers.rb:20:in `vite_legacy_typescript_tag'

My app => app/views/layouts/login.html.haml:31

Screenshots 馃摲

Provide console or browser screenshots of the problem.

@StefSchenkelaars StefSchenkelaars added the bug: pending triage Something doesn't seem to be working, but hasn't been verified label Dec 17, 2021
@ElMassimo ElMassimo added bug Something isn't working and removed bug: pending triage Something doesn't seem to be working, but hasn't been verified labels Dec 17, 2021
@ElMassimo
Copy link
Owner

Hi Stef! Thanks for providing a detailed report!

I'll follow up with a comment in the pull request, thanks!

ElMassimo pushed a commit that referenced this issue Dec 17, 2021
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
@ElMassimo
Copy link
Owner

ElMassimo commented Dec 17, 2021

Released vite_ruby@3.0.4 and vite_plugin_legacy@3.0.1.

@khalwat
Copy link

khalwat commented Dec 21, 2021

Are we sure this isn't a regression/mistake in Vite? \u0000 is the Unicode character for null which makes me think it could potentially be a mistake on their end?

I see your link where the modules are prefaced with the id as per: https://github.com/vitejs/vite/blob/e19ba6a98d85586d1f54e6b0eed11dcf72e6c651/docs/guide/api-plugin.md#conventions

...but I'm not sure that's what is going on here, because that's 0 as an id is a different thing than null

@StefSchenkelaars
Copy link
Contributor Author

Well I'm not a sure but when I started on fixing this issue in vite itself, I found that there was more code surrounding this and not just a simple typo. I created a fix within vite but I wasn't really sure on how to properly write a test / spec for this. If you look at the commit that introduced the character, it is also explicitly checked in the build step. But I agree with you that the difference between the \u0000 character and the 0 are indeed interesting.

So in short: I'm not sure but it works 馃槄

@StefSchenkelaars
Copy link
Contributor Author

@khalwat I created a PR for my commit anyway at vite. Mainly to get the discussion going there ;)
See: vitejs/vite#6208

@khalwat
Copy link

khalwat commented Dec 21, 2021

hmmmm you're right, this does look done on purpose: vitejs/vite@3127219

But I wonder if something isn't amiss with, however \0 gets translated into \u0000 during the build process, because the latter is what ends up in the manifest.json

I'd think that this is what we should end up using:

const polyfillId = '\0vite/legacy-polyfills'

Rather than:

\u0000vite/legacy-polyfills

@khalwat
Copy link

khalwat commented Dec 21, 2021

In vite 2.7, the name of the legacy-polyfills entry has changed from "../../vite/legacy-polyfills" to "../../\u0000vite/legacy-polyfills". This filename has changed since this polyfill is a so called virtual file (as explained in the plugin api docs).

One thing to note from the API docs is that it says:

Internally, plugins that use virtual modules should prefix the module ID with \0 while resolving the id, a convention from the rollup ecosystem. This prevents other plugins from trying to process the id (like node resolution), and core features like sourcemaps can use this info to differentiate between virtual modules and regular files. \0 is not a permitted char in import URLs so we have to replace them during import analysis. A \0{id} virtual id ends up encoded as /@id/x00{id} during dev in the browser. The id will be decoded back before entering the plugins pipeline, so this is not seen by plugins hooks code.

So this id prefixing is internal and should be undone before it is saved out to the manifest.json I'd assume. Sounds like something isn't quite right with this round-trip, at least as far as productions builds with manifest: true are concerned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants