Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Decode req.params #417

Closed
Rich-Harris opened this issue Sep 3, 2018 · 6 comments · Fixed by #419
Closed

Decode req.params #417

Rich-Harris opened this issue Sep 3, 2018 · 6 comments · Fixed by #419

Comments

@Rich-Harris
Copy link
Member

Another thing thrown up by the surprisingly productive issue #347: we should probably apply decodeURIComponent to req.params. That's what Express does if you have something like this:

app.get('/blog/:slug', (req, res, next) => {
  // req.params.slug is decoded
});

I think it's what I'd expect to happen in any case.

Rich-Harris added a commit that referenced this issue Sep 3, 2018
@mrkishi
Copy link
Member

mrkishi commented Sep 3, 2018

Sapper should probably decode req.query keys and values as well -- and also replace + with ' ' in these.

@Conduitry
Copy link
Member

(I haven't checked any code or behavior before writing this comment, but) Wouldn't the underlying Express/Polka/etc. library be responsible for decoding req.query?

@mrkishi
Copy link
Member

mrkishi commented Sep 3, 2018

I meant the client-side routing part.

But you bring a good point: currently, server-side rendered components receive the framework's req.query, so I believe that yeah, we really should match that behavior in the client-side router.

@lukeed
Copy link
Member

lukeed commented Sep 4, 2018

Express & Polka both use parseurl which immediately passes off the request to url.parse as soon as there's a query involved, as well as a few other characters, which i know decodes the path. Polka passes off any query key from that result into querystring.parse which I'm 95% sure decodes.

@Rich-Harris
Copy link
Member Author

I've updated the PR so that req.query is consistent between client and server. I hadn't come across the + thing before, interesting

@mylastore
Copy link

does anyone knows if express-validation can some how be added? Is there any discussion forums for svelte or sapper?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants