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

catch-all routing with an _all.js #223

Closed
wants to merge 3 commits into
base: master
from

Conversation

4 participants
@evantahler
Contributor

evantahler commented Nov 7, 2016

Part of the solve for #25
re: #25 (comment)

From the README update:

If you are building an application which requires a catch-all route (for example, /user/4), you can define a _all.js file in the ./pages/user folder. Any un-matched route under the "user" route will be sent to this page.

@evantahler evantahler changed the title from catch-all routing with _all.js to catch-all routing with an _all.js Nov 7, 2016

@evantahler

This comment has been minimized.

Show comment
Hide comment
@evantahler

evantahler Nov 7, 2016

Contributor

Some conversation about this PR from Slack https://zeit-community.slack.com/archives/next/p1478557230000387


TLDR;

  • there's some discussions around this topic already: https://github.com/zeit/next.js/issues?utf8=%E2%9C%93&q=parameter
  • this PR seems like the minimum changeset that would get this kind of thing working
    • NExt already populates this.props.url, which makes extracting whatever you need from the URL very simple
    • some of the other proposals introduce a lot of conditional logic
  • I don't personally agree that routing data needs to be determined from the file structure
    • you shouldn't have more than one type of thing under the namespace
    • both /user/:id and /user/:name should not both be allowed

so if that's the world we live in, and we have server-side getInitialProps to check for existence/auth/etc ... this should actually be all we need!

Contributor

evantahler commented Nov 7, 2016

Some conversation about this PR from Slack https://zeit-community.slack.com/archives/next/p1478557230000387


TLDR;

  • there's some discussions around this topic already: https://github.com/zeit/next.js/issues?utf8=%E2%9C%93&q=parameter
  • this PR seems like the minimum changeset that would get this kind of thing working
    • NExt already populates this.props.url, which makes extracting whatever you need from the URL very simple
    • some of the other proposals introduce a lot of conditional logic
  • I don't personally agree that routing data needs to be determined from the file structure
    • you shouldn't have more than one type of thing under the namespace
    • both /user/:id and /user/:name should not both be allowed

so if that's the world we live in, and we have server-side getInitialProps to check for existence/auth/etc ... this should actually be all we need!

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Nov 7, 2016

Contributor

This is an interesting approach. I need to think about it more.

Contributor

rauchg commented Nov 7, 2016

This is an interesting approach. I need to think about it more.

@evantahler

This comment has been minimized.

Show comment
Hide comment
@evantahler

evantahler Nov 8, 2016

Contributor

As a note, It could also make sense to default the "catch-all" file to index.js, should that exist within a directory. The name _all.js is totally arbitrary.

Contributor

evantahler commented Nov 8, 2016

As a note, It could also make sense to default the "catch-all" file to index.js, should that exist within a directory. The name _all.js is totally arbitrary.

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Nov 8, 2016

Contributor

Also, considering we already support pathname in getInitialProps (https://github.com/zeit/next.js/releases/tag/1.1.0) that sounds pretty smart

Contributor

rauchg commented Nov 8, 2016

Also, considering we already support pathname in getInitialProps (https://github.com/zeit/next.js/releases/tag/1.1.0) that sounds pretty smart

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Nov 8, 2016

Contributor

As in, to extract the "fancy parameter", you'd just have to pathname.split('/').pop()

Contributor

rauchg commented Nov 8, 2016

As in, to extract the "fancy parameter", you'd just have to pathname.split('/').pop()

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Nov 8, 2016

Contributor

My only concern is how we handle this client-side with <Link />. Specially for nested stuff like:

/users/:id/posts
Contributor

rauchg commented Nov 8, 2016

My only concern is how we handle this client-side with <Link />. Specially for nested stuff like:

/users/:id/posts
@evantahler

This comment has been minimized.

Show comment
Hide comment
@evantahler

evantahler Nov 8, 2016

Contributor

@rauchg You just saved 2 lines of code with pathname.split('/').pop(), cool!
(updated)

Contributor

evantahler commented Nov 8, 2016

@rauchg You just saved 2 lines of code with pathname.split('/').pop(), cool!
(updated)

@evantahler

This comment has been minimized.

Show comment
Hide comment
@evantahler

evantahler Nov 8, 2016

Contributor

regarding /users/:id/posts... If you are using the file-system only, I guess that isn't possible (save a lot of symlinks). We should clarify that the wildcard can only be the final part of the route.

That's why I prefer to call this a "catch all" instead of any fancier type of regexp-style routing, because it would be misleading

Contributor

evantahler commented Nov 8, 2016

regarding /users/:id/posts... If you are using the file-system only, I guess that isn't possible (save a lot of symlinks). We should clarify that the wildcard can only be the final part of the route.

That's why I prefer to call this a "catch all" instead of any fancier type of regexp-style routing, because it would be misleading

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Nov 8, 2016

Contributor

Indeed. So I have to think about whether this behavior is worth it. Otherwise, every single folder will stop handling 404s. In that case, explicit _all.js is better

Contributor

rauchg commented Nov 8, 2016

Indeed. So I have to think about whether this behavior is worth it. Otherwise, every single folder will stop handling 404s. In that case, explicit _all.js is better

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Nov 8, 2016

Contributor

But we also need to think about how to handle 404 elegantly from the catch-all. Like, let's say the user id is not found. How do you handle that?

Contributor

rauchg commented Nov 8, 2016

But we also need to think about how to handle 404 elegantly from the catch-all. Like, let's say the user id is not found. How do you handle that?

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Nov 8, 2016

Contributor

But ultimately I guess that's not related to this PR. Since we would still want that for /user?id=xxxx. cc @nkzawa

Contributor

rauchg commented Nov 8, 2016

But ultimately I guess that's not related to this PR. Since we would still want that for /user?id=xxxx. cc @nkzawa

@evantahler

This comment has been minimized.

Show comment
Hide comment
@evantahler

evantahler Nov 8, 2016

Contributor

Yeah, I agree that fancier error handling is a good idea, but should be a more global update (and not located to just this PR)

You've already got support for creating a 500, and it is pretty easy :D

render () {
    if(this.state.user_id > 5){
      throw 500
    }

    return <div>
      <p>user_id: {this.state.user_id}</p>
      <Link href={'/'}>Back</Link>
    </div>
  }

Perhaps we can throw numbers as HTML error-codes?

Contributor

evantahler commented Nov 8, 2016

Yeah, I agree that fancier error handling is a good idea, but should be a more global update (and not located to just this PR)

You've already got support for creating a 500, and it is pretty easy :D

render () {
    if(this.state.user_id > 5){
      throw 500
    }

    return <div>
      <p>user_id: {this.state.user_id}</p>
      <Link href={'/'}>Back</Link>
    </div>
  }

Perhaps we can throw numbers as HTML error-codes?

@jaredpalmer

This comment has been minimized.

Show comment
Hide comment
@jaredpalmer

jaredpalmer Nov 14, 2016

Contributor

regarding /users/:id/posts... If you are using the file-system only, I guess that isn't possible (save > a lot of symlinks). We should clarify that the wildcard can only be the final part of the route.

@rauchg @evantahler

instead of _all.js, could you designate wildcards with folder names instead? 🤔

// equivalent to /users/:id/posts/:title
.
└── pages
    ├── index.js
    └── users
        ├── _id
        │   ├── edit.js
        │   ├── index.js
        │   └── posts
        │       ├── _title
        │       │   ├── edit.js
        │       │   └── index.js
        │       ├── index.js
        │       └── new.js
        ├── index.js
        └── new.js

Here's how you could mimic a wordpress blog's permalink structure (with paginated archives on year and month):

// equivalent to 
// example.com
// example.com/:year
// example.com/:year/page/:number
// example.com/:year/:month
// example.com/:year/:month/page/:number  
// example.com/:year/:month/:title
.
└── pages
    ├── _year
    │   ├── _month
    │   │   ├── _title
    │   │   │   └── index.js
    │   │   ├── index.js
    │   │   └── page
    │   │       └── _number
    │   │           └── index.js
    │   ├── index.js
    │   └── page
    │       └── _number
    │           └── index.js
    └── index.js

This would cover all use cases i can think of, the only thing this doesn't allow for (out-of-the-box at least) is a medium-like permalink structure: medium.com/@:user_id. However, depending on how you walk the folders / parse this could also be done, but would probably require a different special character as underscore is pretty popular in URLs. Double underscore or dashes would nicely parallel BEM CSS modifiers. Could also pay some homage to php with $.

Contributor

jaredpalmer commented Nov 14, 2016

regarding /users/:id/posts... If you are using the file-system only, I guess that isn't possible (save > a lot of symlinks). We should clarify that the wildcard can only be the final part of the route.

@rauchg @evantahler

instead of _all.js, could you designate wildcards with folder names instead? 🤔

// equivalent to /users/:id/posts/:title
.
└── pages
    ├── index.js
    └── users
        ├── _id
        │   ├── edit.js
        │   ├── index.js
        │   └── posts
        │       ├── _title
        │       │   ├── edit.js
        │       │   └── index.js
        │       ├── index.js
        │       └── new.js
        ├── index.js
        └── new.js

Here's how you could mimic a wordpress blog's permalink structure (with paginated archives on year and month):

// equivalent to 
// example.com
// example.com/:year
// example.com/:year/page/:number
// example.com/:year/:month
// example.com/:year/:month/page/:number  
// example.com/:year/:month/:title
.
└── pages
    ├── _year
    │   ├── _month
    │   │   ├── _title
    │   │   │   └── index.js
    │   │   ├── index.js
    │   │   └── page
    │   │       └── _number
    │   │           └── index.js
    │   ├── index.js
    │   └── page
    │       └── _number
    │           └── index.js
    └── index.js

This would cover all use cases i can think of, the only thing this doesn't allow for (out-of-the-box at least) is a medium-like permalink structure: medium.com/@:user_id. However, depending on how you walk the folders / parse this could also be done, but would probably require a different special character as underscore is pretty popular in URLs. Double underscore or dashes would nicely parallel BEM CSS modifiers. Could also pay some homage to php with $.

@rangerscience

This comment has been minimized.

Show comment
Hide comment
@rangerscience

rangerscience Nov 21, 2016

@jaredpalmer Doesn't the solution run into issues if there's more than one folder? If I have both pages/foo/month and pages/bar/month, which does a request to 6/month get routed to? @evantahler , same goes for the :id.js syntax. Seems to me that the catch-all has to be a specific name (like _all.js) so that it's impossible to accidentally define more than one.

Seems like you'd want something like pages/posts/_all.js catching posts/6 and pages/posts/_all/monthcatchingposts/6/month`.

rangerscience commented Nov 21, 2016

@jaredpalmer Doesn't the solution run into issues if there's more than one folder? If I have both pages/foo/month and pages/bar/month, which does a request to 6/month get routed to? @evantahler , same goes for the :id.js syntax. Seems to me that the catch-all has to be a specific name (like _all.js) so that it's impossible to accidentally define more than one.

Seems like you'd want something like pages/posts/_all.js catching posts/6 and pages/posts/_all/monthcatchingposts/6/month`.

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Nov 21, 2016

Contributor

I remain unconvinced of these smart handlers. We are just going to make the
server API really easy to use and provide great examples of custom routing
:)

On Mon, Nov 21, 2016 at 6:29 AM narfanator notifications@github.com wrote:

@jaredpalmer https://github.com/jaredpalmer Doesn't the solution run
into issues if there's more than one folder? If I have both
pages/foo/month and pages/bar/month, which does a request to 6/month get
routed to? @evantahler https://github.com/evantahler , same goes for
the :id.js syntax. Seems to me that the catch-all has to be a specific
name (like _all.js) so that it's impossible to make multiple catch-alls

Seems like you'd want something like pages/posts/_all.js catching posts/6
and pages/posts/_all/monthcatchingposts/6/month`.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#223 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAy8XC7kXG-Av_z8oG3J7VE2N0BxQvUks5rAWR8gaJpZM4Krw5O
.

Contributor

rauchg commented Nov 21, 2016

I remain unconvinced of these smart handlers. We are just going to make the
server API really easy to use and provide great examples of custom routing
:)

On Mon, Nov 21, 2016 at 6:29 AM narfanator notifications@github.com wrote:

@jaredpalmer https://github.com/jaredpalmer Doesn't the solution run
into issues if there's more than one folder? If I have both
pages/foo/month and pages/bar/month, which does a request to 6/month get
routed to? @evantahler https://github.com/evantahler , same goes for
the :id.js syntax. Seems to me that the catch-all has to be a specific
name (like _all.js) so that it's impossible to make multiple catch-alls

Seems like you'd want something like pages/posts/_all.js catching posts/6
and pages/posts/_all/monthcatchingposts/6/month`.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#223 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAy8XC7kXG-Av_z8oG3J7VE2N0BxQvUks5rAWR8gaJpZM4Krw5O
.

@evantahler

This comment has been minimized.

Show comment
Hide comment
@evantahler

evantahler Nov 21, 2016

Contributor

@rauchg so what is the path forward here? I can add an _all to the middle of routes as well, or should I just leave this as-is for catch-all files?

Contributor

evantahler commented Nov 21, 2016

@rauchg so what is the path forward here? I can add an _all to the middle of routes as well, or should I just leave this as-is for catch-all files?

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Nov 23, 2016

Contributor

I'm documenting the path forward with this in an issue which I should have ready within a day or two

Contributor

rauchg commented Nov 23, 2016

I'm documenting the path forward with this in an issue which I should have ready within a day or two

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Nov 23, 2016

Contributor
Contributor

rauchg commented Nov 23, 2016

@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