-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 server crash when accessing a malformed URI #5246
fix server crash when accessing a malformed URI #5246
Conversation
🦋 Changeset detectedLatest commit: be026cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
I'm not sure how to fix the test. I would need some help there. |
@ivanhofer I'm not sure if you've noticed, but the tests appear to be failing with this PR |
Yes I saw. Thats why I left the comment above. I would expect the test to work that I have written. In CI two tests are failing. Locally only the test with |
Ah, sorry, I didn't see your comment. It seems like the failure is coming from Vite. Probably the best course of action would be to create a small project that reproduces this using only Vite and not SvelteKit and file a bug there. See https://github.com/sveltejs/kit#bug-reporting for more details on that It's not clear to me that this PR actually fixes anything. Is there a reason you think it might? Or should we go ahead and close this? At the very least I would mark it as a draft to make it clearer that it's not in a state to be merged |
I don't think this has anything to do with The fix works. We are using it since a few weeks in two of our applications. |
The 500 on the CI is coming from Vite according to the stack trace. Did you test this fix with your own app in both dev and build mode? The Vite issue is only occuring in dev mode since that's where the Vite dev server is used |
Ah, now I see that there are two different "test" calls "test:dev" and "test:prod". So there is actually a bug in SvelteKit and in vite. I'll also open an issue there. Still there is an error when running "test:prod" with the |
I took a look at filing a Vite issue this morning, but I think Vite's mostly fine because the server doesn't crash entirely like the SvelteKit server does (although it could arguably return a 400 status code instead of 500). I think we just need to update the test in this PR |
Smart way to handle the test 👍 |
thanks! |
Fix for #5090
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. All changesets should bepatch
until SvelteKit 1.0