veliovgroup / flow-router

🚦 Carefully extended flow-router for Meteor
https://packosphere.com/ostrio/flow-router-extra
BSD 3-Clause "New" or "Revised" License
202 stars 29 forks source link

Dash in paths not recognized as separators #25

Closed nicooprat closed 7 years ago

nicooprat commented 7 years ago

I had this kind of routes working with original FlowRouter: /@:username/:storyId-:storySlug?.

Now, the second part is considered to be a unique block :storyId-:storySlug? according to this regex:

https://github.com/VeliovGroup/flow-router/blob/110a27945417f9fbc45a6262d7245a9335d1c704/client/router.js#L8

Removing the \- makes it work again: https://regex101.com/r/CadP4l/2.

Is it something done on purpose? Is there any other recommended way of defining routes?

Thanks.

dr-dimitru commented 7 years ago

Hello @nicooprat ,

Is it something done on purpose?

Yes. Take a look on this commit and this PR.

As this was done almost a year ago, not sure if it's safe to roll back.

Is there any other recommended way of defining routes? Yes, split each param using forward slash /

WDYT?

nicooprat commented 7 years ago

I understand. Pretty hard for me too to change all my URIs :D And I think it's more meaningful in my case to configure path like this ; would be weird to have id/slug...

Would it be possible to make this regex configurable? Or better, an array of "URI delimiters"?

By the way, are optional params still a thing? In this case: /@:username/:storyId/:storySlug?/:chapterId/:chapterSlug?, what would happen if storySlug is missing?

dr-dimitru commented 7 years ago

Pretty hard for me too to change all my URIs :D And I think it's more meaningful in my case to configure path like this ; would be weird to have id/slug...

We're okay to use slug after slash, for example:

Would it be possible to make this regex configurable? Or better, an array of "URI delimiters"?

This is a good idea, marked this thread as [enhancement]. PR is welcome, or we will add this feature at the next release.

By the way, are optional params still a thing?

Yes

/@:username/:storyId/:storySlug?/:chapterId/:chapterSlug? what would happen if storySlug is missing?

storySlug can not be omitted as chapterId is required and should optional too for this case.

nicooprat commented 7 years ago

Made the simplest change I could think of: https://github.com/VeliovGroup/flow-router/pull/26

I think it would be better to create a setter function taking an array of strings as parameters... But I'm really not good enough in Regex :)

nicooprat commented 7 years ago

Thanks! Do you plan to bump the package version & publish on Atmosphere soon?

dr-dimitru commented 7 years ago

@nicooprat yes, working on it now. Too many packages are connected. Decided to have a sleep before sending it to Package Manager :)

nicooprat commented 7 years ago

👍 thanks!

dr-dimitru commented 7 years ago

@nicooprat does it work for you?

nicooprat commented 7 years ago

Yes, the override is working. Didn't see any bug. Thanks again!