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.

Sign up for GitHub

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: handle new virtual legacy-polyfill name #157

Merged
merged 1 commit into from
Dec 17, 2021
Merged

fix: handle new virtual legacy-polyfill name #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions test/test_app/public/vite-production/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
"_log.d31acc25-legacy.js": {
"file": "assets/log.d31acc25-legacy.js"
},
"../../vite/legacy-polyfills": {
"../../\u0000vite/legacy-polyfills": {
"file": "assets/polyfills-legacy.07477394.js",
"src": "../../vite/legacy-polyfills",
"isEntry": true
Expand Down Expand Up @@ -100,4 +100,4 @@
"_log.818edfb8.js": {
"file": "assets/log.818edfb8.js"
}
}
}
6 changes: 4 additions & 2 deletions vite_ruby/lib/vite_ruby/manifest.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Owner

@ElMassimo ElMassimo Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could take this opportunity to generalize the check:

unless name.include?("\u0000")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@ElMassimo ElMassimo Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

name = with_file_extension(name.to_s, type)

raise ArgumentError, "Asset names can not be relative. Found: #{ name }" if name.start_with?('.') && !name.include?('legacy-polyfills')
raise ArgumentError, "Asset names can not be relative. Found: #{ name }" if name.start_with?('.')
end

# Explicit path, relative to the source_code_dir.
name.sub(%r{^~/(.+)$}) { return Regexp.last_match(1) }
Expand Down