riot / route

Simple isomorphic router
MIT License
212 stars 40 forks source link

use decodeURI() on route #155

Closed geonanorch closed 3 years ago

geonanorch commented 3 years ago

Hi, and thank you for the new Riot.js 5.0 version, which indeed seems totally backward-compatible :-)

I just hit the issue reported in #29 and #57: I assumed that whatever values were provided by route were already decoded and ended with a mismatch against application data as one of the path element contained a space.

So instead of writing:

        <route path="/builder/:sitename?(/?[?#].*)?(#.*)?">
          <my-builder-page sitename="{ route.params.sitename }"></my-builder-page>
        </route>

I have to write:

        <route path="/builder/:sitename?(/?[?#].*)?(#.*)?">
          <my-builder-page sitename="{ decodeURIComponent(route.params.sitename) }"></my-builder-page>
        </route>

The old issues mentioned above refered to a spec, which is no longer available, so I cannot dig deeper. However, barring a clear reason against it, I think that it would more natural and added value to provided decoded information --that is typically the sort of annoying details, which frameworks take care of. This should not be a breaking change (or just in border cases where the decoded data is again decodable).

GianlucaGuarini commented 3 years ago

Thanks to the use of erre streams this issue can be simply solved with 1 line of code:

import { router } from '@riotjs/route'

// make sure all strings passing through the router stream will be decoded
router.connect(decodeURIComponent)

The problem here is that decodeURIComponent is not available in node so I have tried to keep @riotjs/route polymorphic for the client and server. Please let me know if the snipped above worked for you

geonanorch commented 3 years ago

Sorry for the delay in answering, holidays got in the way... Thank you for the tip, which looks pretty elegant. I will try it out in the coming days then either close this issue or report my findings. Just a remark: since decoded data is the expected behavior (I am referring to my own experience + the issues I mentioned in the description), would it not be better to define a polyfill (even if just a dummy / identity function) for decodeURIComponent() which would kick in under node ?

geonanorch commented 3 years ago

I tested your suggestion and it works fine for my case, thank you 😄

Since I still believe that this issue should be fixed within riot (by providing a dummy for Node.js, see my previous comment) I will leave the ticket open, decision is yours!

GianlucaGuarini commented 3 years ago

@geonanorch the issue should be fixed in https://github.com/gianlucaguarini/rawth since this package depends on it

geonanorch commented 3 years ago

@GianlucaGuarini not sure I understand, do you mean that you would like me to raise an issue against rawth ?

By the way decodeURIComponent is available under Node.js and is defined globally.