search

QubitProducts / cherrytree

A flexible nested router.
MIT License
108 stars 20 forks source link

Strip slashes from the end of generated url #150

Closed KidkArolis closed 6 years ago

KidkArolis commented 8 years ago

paths like /path/:a?/:b?/:c? with no parameters, generates /path/// and causing error "TypeError: Cannot read property 'options' of undefined"

This PR builds on #129 .

swagatha-christie commented 8 years ago

By analyzing the blame information on this pull request, we identified @nathanboktae to be a potential reviewer

KidkArolis commented 8 years ago

Hey @xrado, sorry for the radio silence for the entire year, lol.

If this is still relevant or you're still interested in this issue, I was just looking over it. One thing I noticed after adding some more tests is this behaviour:

assert.equals(Path.injectParams('/foo/:a?/:b?/:c?', {c: 1}), '/foo///1')

That is, when generating URL with such optional params, it produces /foo///1. The issue now is this URL is not recognised anymore, that is Path.extractParams('/foo/:a?/:b?/:c?', '/foo///1') returns null.

And the thing is, I'm not sure what the behaviour should be, seems quite confusing either way. E.g. if we produce /foo/1 for Path.injectParams('/foo/:a?/:b?/:c?', {c: 1}), then when matching, that param now maps back to a, not c.

I'm curious about what use cases these optional URLs are useful for? In those cases, wouldn't it be better to just map it to a :splat*? Basically, my advice is to avoid such urls patterns :)

Ok, so to sum this up, we have the following options:

  1. Keep the implementation as is on master - keeps things simple, might be best to avoid optional params in the middle of the URL.
  2. Merge this PR as it currently stands - the issue with that is that you can still generate URLs like /foo////c, which isn't very useful, because they can't be matched by the router.
  3. Continue working on this PR, figure out how we want to handle this /foo////c case - we could make the route match this URL, or convert it to /foo/c.
xrado commented 8 years ago

url path like /foo////c ..may be valid, but looks somehow wrong or a bad design to me. I'm looking it like a filesystem folders. You can access a folder with no name.

just my opinion

KidkArolis commented 8 years ago

How can you access a folder with no name? I thought you can't create or access folders with no names?

xrado commented 8 years ago

ofcourse not, /foo////c is like accessing subfolders of foo that do not exists and "c" is floating somewhere

KidkArolis commented 6 years ago

Don't have enough knowledge about this issue in my head to action further. Reopen as issue / PR if it's still relevant.

grajagandev commented 6 years ago

Would you be interested in software that automatically finds TypeErrors caused by corner cases like this extra slashes in the URL? I am building Fuzz Stati0n to do that (free for OSS) - please take a look and consider signing up for our newsletter to keep in touch.