fix(router-core): Backslash in path parameters are incorrectly restored from URL #3987
+4
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
View your CI Pipeline Execution ↗ for commit 146c0ac.
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/create-router
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/create-start
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-with-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-config
@tanstack/react-start-plugin
@tanstack/react-start-router-manifest
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-config
@tanstack/solid-start-plugin
@tanstack/solid-start-router-manifest
@tanstack/solid-start-server
@tanstack/start
@tanstack/start-api-routes
@tanstack/start-client-core
@tanstack/start-config
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-handler
@tanstack/start-server-functions-server
@tanstack/start-server-functions-ssr
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
|
can you please remove the irrelevant commits from this PR? |
leesb971204
force-pushed
the
fix/backshlash-path-parameters
branch
from
62b0742 to
0c7985e
Compare
github-actions
bot
added
documentation
leesb971204
force-pushed
the
fix/backshlash-path-parameters
branch
from
0c7985e to
62b0742
Compare
github-actions
bot
removed
documentation
github-actions
bot
added
package: router-core
and removed
package: router-plugin
package: start
package: start-config
package: router-generator
package: router-cli
package: valibot-adapter
package: react-router-with-query
package: router-devtools
package: create-start
package: zod-adapter
package: create-router
package: arktype-adapter
package: router-vite-plugin
package: start-api-routes
package: start-server-functions-ssr
package: start-server-functions-fetcher
package: start-server-functions-handler
package: start-server-functions-client
labels
leesb971204
force-pushed
the
fix/backshlash-path-parameters
branch
from
62b0742 to
51fff1a
Compare
…ecoding logic Signed-off-by: leesb971204 <leesb971204@gmail.com>
…URI encoded characters. Signed-off-by: leesb971204 <leesb971204@gmail.com>
|
I think we need to write separate tests for paths containing special characters, depending on whether we use |
There was a problem hiding this comment.
Aside of the test regressions present in the unit testing, I'm not sure if this is the right approach.
I'm wondering whether we should be doing any decoding of the named path parameters (not the splat parameter) at all. Perhaps, we shouldn't be encoding the path parameters at all? 🤔
Whilst, URI encoding works for the URL Search Parameters, perhaps we shouldn't be doing this for the path parameters. Essentially we'd leave them "as-is".
// path.ts
value: part.includes('%25')
? part
.split('%25')
.map((segment) => decodeURI(segment))
.join('%25')
: hasUriEncodedChars(part)
? part
: decodeURI(part),
👇
value: partSo do you think we should make the change as described above? |
|
Yea, I'm wondering if that'd be the trick. Also, whether or not we should be doing the Nothing concrete. Just thinking of ideas to go about this without breaking everything for everyone 😅 |
Exactly. The decoding logic does seem pretty complex. For encoding, I think it might be reasonable to delegate that responsibility to the user when it comes to path parameters. But if we go that route, it would likely introduce a lot of changes to the existing codebase. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes: #3962
Please ignore the commits before the 1c3303b, as they include work related to another PR.
I believe the bug is caused by an unnecessary decodeURI in
path.ts.It works well, but I think we need to reconsider the test case we wrote. I think the test case is incorrectly written.
If my understanding is incorrect, I apologize.