Skip to content
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 from
Closed

catch-all routing with an _all.js #223

wants to merge 3 commits into from

Conversation

evantahler
Copy link
Contributor

@evantahler 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 catch-all routing with _all.js catch-all routing with an _all.js Nov 7, 2016
@evantahler
Copy link
Contributor Author

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
Copy link
Member

rauchg commented Nov 7, 2016

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

@evantahler
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

rauchg commented Nov 8, 2016

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

@rauchg
Copy link
Member

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
Copy link
Contributor Author

evantahler commented Nov 8, 2016

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

@evantahler
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
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 $.

@rangerscience
Copy link

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Member

rauchg commented Nov 23, 2016

#291

@rauchg rauchg closed this Nov 23, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants