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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: escape vite asset import suffixes #46

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

barakcodes
Copy link
Contributor

@barakcodes barakcodes commented Dec 3, 2024

Add the regex to exclude all vite asset import suffixes, might be better done in the @hono/vite-dev-server(maybe), but this fixes the issue for now

Rel #44

@barakcodes barakcodes force-pushed the fix-vite-asset-import branch 2 times, most recently from c799af0 to cb85414 Compare December 3, 2024 20:06
@john-griffin
Copy link
Contributor

Thanks, I checked this out locally and I still see the 404 in the example you updated.

@barakcodes
Copy link
Contributor Author

Thanks, I checked this out locally and I still see the 404 in the example you updated.

Hmm, it works for me. I look more into it

@barakcodes barakcodes force-pushed the fix-vite-asset-import branch from cb85414 to 28e9b2d Compare December 4, 2024 07:17
@barakcodes
Copy link
Contributor Author

Thanks, I checked this out locally and I still see the 404 in the example you updated.

@john-griffin could you give it another try?

@john-griffin
Copy link
Contributor

Still a 404 with that pattern. I'm running via the npm run dev command if that makes any difference, not npm run start-with-adapter

@barakcodes
Copy link
Contributor Author

Still a 404 with that pattern. I'm running via the npm run dev command if that makes any difference, not npm run start-with-adapter

shoot! I'll take a deeper look at this over the weekend

src/vite-plugin.ts Outdated Show resolved Hide resolved
Add the regix to exclude all vite asset import suffixes,
might be better done in the @hono/vite-dev-server, but this
fixes the issue for now
@barakcodes barakcodes force-pushed the fix-vite-asset-import branch from 28e9b2d to f309859 Compare December 5, 2024 17:36
@barakcodes
Copy link
Contributor Author

barakcodes commented Dec 5, 2024

So

Still a 404 with that pattern. I'm running via the npm run dev command if that makes any difference, not npm run start-with-adapter

turns out I was testing with ?inline which did not have?import prefix in some cases for some reason. this should now work as it matches both ?inline and ?import&inline and similar combinations

@john-griffin
Copy link
Contributor

Thanks, that pattern is now working, imports are being returned with a 200 👍

@barakcodes barakcodes requested a review from yusukebe December 5, 2024 18:55
@yusukebe
Copy link
Owner

yusukebe commented Dec 6, 2024

Hey @barakcodes

Thank you for fixing. I've added the test for this change.

Copy link
Owner

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe
Copy link
Owner

yusukebe commented Dec 6, 2024

Looks good! Let's land it.

@yusukebe yusukebe merged commit 3e1ee79 into yusukebe:main Dec 6, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants