gvergnaud / nextjs-dynamic-routes

[Deprecated] Super simple way to create dynamic routes with Next.js
MIT License
140 stars 7 forks source link

Optional path segments almost working #1

Closed kyen99 closed 6 years ago

kyen99 commented 6 years ago

I'm trying to use this with a path that has an optional path segment like this...

router.add({ name: 'company', pattern: '/company/:slug/:view?' })

The route works but generating routes with Link or Router doesn't quite work as expected. Currently, this is what happens...

Not including optional param: Router.pushRoute('company', { slug='google' } results in /company/google/:view?

Including optional param as an empty string: Router.pushRoute('company', { slug='google', view='' } results in/company/google/?`

Fortunately, the latter works well enough for me, but I would expect both to simply return /company/google

I'm not sure if you were intending support for this. It looks like the regex at the end of replaceWithParams is trying to do something like this, but doesn't quite match :view? as in my example above.

export const replaceWithParams = (pattern, params) =>
  Object.keys(params)
    .reduce((acc, param) => acc.replace(`:${param}`, params[param]), pattern)
    .replace(/:[^\/&\?]*(\/|$)/g, '')

I wanted to find out if you have any interest in supporting this before submitting a PR. Also, before doing so I wanted to make sure I fully understand what that regex is supposed to do.

gvergnaud commented 6 years ago

Hey,

for this kind of edge cases, replaceWithParams doesn't work exactly as expected. Actually I was planning on refactoring this with path-to-regexp, but I didn't find the time to do that yet.

If you want to propose a local solution for this problem, of course you are welcome to open a PR!

honzasusek commented 6 years ago

@kyen99 @gvergnaud Fixed in https://github.com/gvergnaud/nextjs-dynamic-routes/pull/4

gvergnaud commented 6 years ago

Fixed it in 2.1.8