New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for parameterized routing. Refs #59, #91 #142

Closed
wants to merge 5 commits into
base: master
from

Conversation

5 participants
@dstreet
Contributor

dstreet commented Oct 28, 2016

Pulls routes from next.config.js:

// next.config.js

exports.routes = [
  { path: '/posts/:id', file: 'posts/post' }
]
pages\
    index.js
routes\
    posts\
        post.js

Routes defined this way are resolved to the routes directory rather than pages to avoid conflicts and ensure existing functionality remains intact. Additionally, a parameters map is passed to each of the components via ctx.

@dstreet

This comment has been minimized.

Show comment
Hide comment
@dstreet

dstreet Oct 28, 2016

Contributor

#91, #59

Contributor

dstreet commented Oct 28, 2016

#91, #59

@nodegin

This comment has been minimized.

Show comment
Hide comment
@nodegin

nodegin Oct 29, 2016

Contributor

This solution is duplicating pages folder! Which isn't minimalistic at all..

Contributor

nodegin commented Oct 29, 2016

This solution is duplicating pages folder! Which isn't minimalistic at all..

@dstreet

This comment has been minimized.

Show comment
Hide comment
@dstreet

dstreet Oct 29, 2016

Contributor

I don't agree. It is meant to be a complement to the pages folder. pages is for static routes, whereas routes is for dynamic routing. If an app has no need for dynamic routing, then this directory can be ignored, maintaining an extremely minimal implementation.

Contributor

dstreet commented Oct 29, 2016

I don't agree. It is meant to be a complement to the pages folder. pages is for static routes, whereas routes is for dynamic routing. If an app has no need for dynamic routing, then this directory can be ignored, maintaining an extremely minimal implementation.

@nkzawa

This comment has been minimized.

Show comment
Hide comment
@nkzawa

nkzawa Oct 29, 2016

Member

@dstreet @nodegin I wonder how you think about #25 which can support this kind of routings too, as external plugins. I think it's more flexible and simple.

Member

nkzawa commented Oct 29, 2016

@dstreet @nodegin I wonder how you think about #25 which can support this kind of routings too, as external plugins. I think it's more flexible and simple.

@nodegin

This comment has been minimized.

Show comment
Hide comment
@nodegin

nodegin Oct 29, 2016

Contributor

@nkzawa Yes I agree providing an external for detailed customizations. But we can also give some basic support in the framework IMHO.

Contributor

nodegin commented Oct 29, 2016

@nkzawa Yes I agree providing an external for detailed customizations. But we can also give some basic support in the framework IMHO.

@dstreet

This comment has been minimized.

Show comment
Hide comment
@dstreet

dstreet Oct 29, 2016

Contributor

@nkzawa I would be ok with that approach as long as plugins could hook into the existing rendering and transpiling.

Contributor

dstreet commented Oct 29, 2016

@nkzawa I would be ok with that approach as long as plugins could hook into the existing rendering and transpiling.

@hoangzinh

This comment has been minimized.

Show comment
Hide comment
@hoangzinh

hoangzinh Nov 20, 2016

I'm looking forward to hoping this PR will be merged to our repo. I need to custom route to do something like:
/photos/:id => pages/photos/photo

hoangzinh commented Nov 20, 2016

I'm looking forward to hoping this PR will be merged to our repo. I need to custom route to do something like:
/photos/:id => pages/photos/photo

@dstreet

This comment has been minimized.

Show comment
Hide comment
@dstreet

dstreet Nov 20, 2016

Contributor

@hoangzinh I think the maintainers are going a different direction.

Contributor

dstreet commented Nov 20, 2016

@hoangzinh I think the maintainers are going a different direction.

@hoangzinh

This comment has been minimized.

Show comment
Hide comment
@hoangzinh

hoangzinh commented Nov 21, 2016

@dstreet ah I see

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Nov 23, 2016

Contributor

@dstreet

Considering #291 (comment), I suggest you start looking into transforming this into an API like:

const nextRoutes from 'next-routes'
const app = next()
createServer(nextRoutes(app)).listen(process.env.PORT || 3000)

Users of this approach will have to use node server.js instead of next start.

Then you can extend next.config.js with your routes, and you can read the config from app.config (cc @nkzawa).

This will give you the great expressiveness and ease-of-use, without us having to bloat the core :)

Contributor

rauchg commented Nov 23, 2016

@dstreet

Considering #291 (comment), I suggest you start looking into transforming this into an API like:

const nextRoutes from 'next-routes'
const app = next()
createServer(nextRoutes(app)).listen(process.env.PORT || 3000)

Users of this approach will have to use node server.js instead of next start.

Then you can extend next.config.js with your routes, and you can read the config from app.config (cc @nkzawa).

This will give you the great expressiveness and ease-of-use, without us having to bloat the core :)

@rauchg rauchg closed this Nov 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment