Fix for searchParams #817
Fix for searchParams #817
Conversation
🦋 Changeset detectedLatest commit: ef1b5a7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
commit: |
| Object.entries(query).forEach(([key, value]) => { | ||
| if (Array.isArray(value)) { | ||
| value.forEach((entry) => urlQuery.append(key, decodeURIComponent(entry))); | ||
| value.forEach((entry) => queryStrings.push(`${key}=${entry}`)); |
There was a problem hiding this comment.
Why not use URLSearchParams here without the decodeURIComponent?
Encoding the search parameters manually seems error-prone
There was a problem hiding this comment.
Ho right i'll revert this one, that was an attempt to fix it, but this function wasn't the only issue here
There was a problem hiding this comment.
I do remember now why i did that, convertToQueryString is only used for already properly encoded search parameters.
The query in these cases are provided by the converter (which in the case of cloudflare already use URLSearchParams)
If we revert this without the decodeURIComponent it will break the handleRewrite from next config (Specifically this test
opennextjs-aws/packages/tests-unit/tests/core/routing/matcher.test.ts
Lines 295 to 303 in e9b37fd
It should be safe enough to use it here as long as we don't use it in places where query could be not properly encoded.
| @@ -41,10 +41,18 @@ export function convertFromQueryString(query: string) { | |||
| return queryParts.reduce( | |||
| (acc, part) => { | |||
| const [key, value] = part.split("="); | |||
| acc[key] = value; | |||
| if (key in acc) { | |||
There was a problem hiding this comment.
Maybe factor with getQueryFromSearchParams by passing [k,v][]?
Fix multivalue query handling in searchParams in Node.
Should fix opennextjs/opennextjs-cloudflare#533
Potentially fix this one as well opennextjs/opennextjs-cloudflare#537