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

context params accessed via ctx.params in the handlers need to be decoded #2310

Open
king8fisher opened this issue Feb 13, 2024 · 3 comments · May be fixed by #2311
Open

context params accessed via ctx.params in the handlers need to be decoded #2310

king8fisher opened this issue Feb 13, 2024 · 3 comments · May be fixed by #2311

Comments

@king8fisher
Copy link
Contributor

Imagine a route file named [product].tsx. When you handle a request, you are reaching for the param named "product". If the product sent contains a space, it comes with a %20 when reaching its value via ctx.params["product"]. The feeling is that it would be more convenient if returning value was already decoded according to how a browser would so that the user wouldn't need to guess which function to utilize.

export const handler: Handlers<ProductsPageProps> = {
  GET(_req, ctx) {
    const productName = decodeURI(ctx.params["product"]);
@deer
Copy link
Contributor

deer commented Feb 13, 2024

Cool idea. I wrote a test like this:

Deno.test("param with encoding", async () => {
  await withFakeServe("./tests/fixture/dev.ts", async (server) => {
    const doc = await server.getHtml(`/decode-params/Hello%20World`);
    assertEquals(
      doc.querySelector("body")?.textContent,
      "Hello World",
    );
  });
});

which invokes:

import { defineRoute } from "$fresh/src/server/defines.ts";

export default defineRoute((req, ctx) => {
  return ctx.params.id;
});

Prior to the change, the test fails. But then I do this:

--- a/src/server/router.ts
+++ b/src/server/router.ts
@@ -189,9 +189,13 @@ export function getParamsAndRoute(
       const res = route.pattern.exec(url);

       if (res !== null) {
+        const decodedParams: Record<string, string | undefined> = {};
+        for (const [key, value] of Object.entries(res.pathname.groups)) {
+          decodedParams[key] = value ? decodeURIComponent(value) : value;
+        }
         return {
           route: route,
-          params: res.pathname.groups,
+          params: decodedParams,
           isPartial,
         };
       }

and the test starts passing 🎉

But I think such a brutal approach would be a breaking change. Who knows what sort of logic people have written with the expectation that things remain encoded. Tomorrow I'll do a more subtle approach involving a new configuration option that allows this to be enabled.

@king8fisher
Copy link
Contributor Author

A possible why behind returning a raw value. decodeURI throws if original value arrives wrongly formatted.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/decodeURI

If decoding is the user's responsibility the they will handle the error.

Alternative would be to return a null for wrongly encoded values.

@deer deer linked a pull request Feb 14, 2024 that will close this issue
@king8fisher
Copy link
Contributor Author

I just realized that I needed to encode my params in the urls too, otherwise whatever is passed may lack some portions of the data containing reserved symbols (for instance, after a # symbol that's part of the passed url).

It must be an overlook that proper constructions of the urls should indeed include an encoder.

<a href={`/products/${encodeURIComponent(product)}`}>

Now the mirrored decoding does not look too strange.

If everything I said is right, would it make sense to add a note somewhere in the docs? If so I will gladly PR this and it wouldn't hurt #2311

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 a pull request may close this issue.

2 participants