-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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] handle paths consistently between dev and various production adapters #2171
Conversation
🦋 Changeset detectedLatest commit: 64087f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
ecc6080
to
647ebff
Compare
fc6b657
to
ff2ef9b
Compare
35836a0
to
c3d0d41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0e758b9
to
64087f8
Compare
Yeah, I think it's the better behavior to return the path as it was received. There are some edge cases that are impossible to handle otherwise. E.g. see this post where Netlify started out giving users the decoded path and had to switch: https://answers.netlify.com/t/bug-fix-url-encoding-preserved-in-function-event/27080. This is also inline with how Express and most other libraries work, so I think it's generally more expected as well |
Ah, gotcha. That makes sense |
fixes #2166
dev and
adapter-node
were passing decoded paths while the other adapters were passing the encoded path sent by the browserThe way to handle the path is to split it on forward slashes and then call
decodeURIComponent
on each path component. That allows you to handle edge cases like%2f
. Luckily, that's whatbuild
is currently doing:kit/packages/kit/src/core/build/index.js
Line 614 in c3ca3fb
Otherwise, we shouldn't touch the path. We should just leave it alone and pass along exactly what we received, which is exactly how Express behaves
Previously there was another bug because we were doing
decodeURI
followed bydecodeURIComponent
which would cause each component to be double-decoded. By leaving the path alone we don't run into this issue