fridays / next-routes

Universal dynamic routes for Next.js
MIT License
2.47k stars 230 forks source link

Link to a path #46

Closed acanimal closed 7 years ago

acanimal commented 7 years ago

In addition to link to a route with params we can also link to a given path that conforms a route pattern.

In my case I have complex route patterns which extracts some variables from the path. I want to be able to Link to a different path and use the power of regexp to extract again the variables from the path instead to compute the route with params.

coluccini commented 7 years ago

:+1: This would be really useful when working with API that returns full paths

acanimal commented 7 years ago

Hi @fridays, this PR is important for me. I would appreciate if you could review and, if accepted, publish a new npm package version.

HaNdTriX commented 7 years ago

Hey @acanimal

Just a short question what exactly is the difference between:

Router.push('/somePath?query=123')
Router.pushPath('/somePath?query=123')
acanimal commented 7 years ago

@HaNdTriX Router.push() will try to find a next page with the given path. In contrast with Router.pushPath we can pass a path and found the router that match that regexp.

In our case we have different routes that must render the same next page. That methods helps us on that tasks.

coluccini commented 7 years ago

For example:

routes.add('someRouteName', '/:path', 'otherPage');

Router.push('/somePath?query=123')
// -> Search por /pages/somePath
Router.pushPath('/somePath?query=123')
// -> Search por /pages/otherPage
fridays commented 7 years ago

Thank you for contributing!

It's great and we should have this feature.

What do you all think about naming it href instead of path as suggested in https://github.com/fridays/next-routes/pull/23, or would it confuse? Then if no matching route was found, we could assign newProps.href and next.js would render it’s error page.

acanimal commented 7 years ago

I agree with the change from path to href. I'll try to made all changes ASAP and update the PR. Thanks.

acanimal commented 7 years ago

@HaNdTriX @fridays Changes done !!! This PR is urgent for us. Once you approved can publish can new package on npm? Thanks in advance

acanimal commented 7 years ago

@HaNdTriX @fridays any news?

fridays commented 7 years ago

Here it's calling this.match(href) but it may be undefined: https://github.com/acanimal/next-routes/blob/a1b8defb43f29b515521f908832501999962f603/src/index.js#L59

I'm not sure if href should take precedence over route, could you share the reasoning?

Another catch: It doesn't yet take the querystring from a given href into account. It has to be parsed and merged with params like in the request handler for consistent results.

acanimal commented 7 years ago

@fridays yes, this.match(href) can be undefined because of this I check on https://github.com/acanimal/next-routes/blob/a1b8defb43f29b515521f908832501999962f603/src/index.js#L64 if match returns some router object then I apply them otherwise use the router property.

There is no special reason why I decide href takes precedence over route. I simply though an href written by the user was more important than a route with params. I think is a more "direct" way to specify so it must take precedence.

Anyway it is not important. I can change if you do not agree.

Totally agree with the fact we are not parsing query params. This little change is really urgent for us. If you agree you could merge to be the changes and I can continue working with the query params.

acanimal commented 7 years ago

@HaNdTriX @fridays any news?

fridays commented 7 years ago

Hi @acanimal I mean href can be undefined, it should not try to match it then.

We should add this feature once complete, see my last comment regarding querystring. Also the other Router methods are not there yet. I can have a look at the missing parts.

If you're in a hurry you can of course npm install your/repo or use a HOC for now