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

Improve "not found" route handling on server #3212

Closed
taion opened this issue Mar 21, 2016 · 11 comments
Closed

Improve "not found" route handling on server #3212

taion opened this issue Mar 21, 2016 · 11 comments
Labels

Comments

@taion
Copy link
Contributor

taion commented Mar 21, 2016

There's an API problem with how we handle "not found" routes.

If you follow the upgrade guide and add a

<Route path="*" component={NotFound} />

Then the pattern we have on the server-side rendering guide won't work for sending a 404 rather than a 200 on a non-matching route, because we'll actually have a match.

IMO it'd be cleanest to have a first-class concept of a "not found" route that can be easily mapped to a 404. A user could otherwise sniff the list of matched routes/components, or else use a side channel for indicating 404 status, but this seems like something we should properly handle.

This will especially matter once we have something like #3098.

@timdorr
Copy link
Member

timdorr commented Mar 21, 2016

Some API ideas:

  • <Route notFound={true} component={NotFound} />
  • <RouteNotFound component={NotFound} />

@taion
Copy link
Contributor Author

taion commented Mar 21, 2016

The first option leads to the currently recommended workaround, which is to do something like

renderProps.routes.some(route => route.notFound)

@ryanflorence
Copy link
Member

@taion
Copy link
Contributor Author

taion commented Mar 22, 2016

That works too. It's possible all we need to do is to just document this in the server rendering guide.

@timdorr
Copy link
Member

timdorr commented Mar 22, 2016

Thought of another one:

  • <Router notFoundComponent={NotFound}> <- In this case, we would have special handling for a matchRoutes miss, which would probably be the most performant option.

@taion
Copy link
Contributor Author

taion commented Mar 22, 2016

If we want router-level support, it ought to be a notFoundRoute rather than a notFoundComponent for consistency and flexibility.

Maybe setting status on a route is good enough, though – among other things it'd work for unauthorized pages, too.

@timdorr
Copy link
Member

timdorr commented Mar 22, 2016

401s and 404s would be tricky to support. You'd not only need to handle a pattern miss, but also keep track of if any of those misses were caused by a conditional function. Not sure how best to handle that. Perhaps some way of signaling from those conditional functions what kind of error it is? Could be a thrown error of a specific type. But I dunno, that is distinctly server-side behavior and this is definitely a client-first library IMHO. I'm not sure how you strike a balance.

In any case, once you figure out what kind of error you've got, it could also be a more generic component that gets an extra error prop. So, it would be <Router errorComponent={Error}> instead.

@timdorr
Copy link
Member

timdorr commented Mar 22, 2016

Or an onError={callback} prop.

@taion
Copy link
Contributor Author

taion commented Mar 22, 2016

It wouldn't be bad at all with something like the pattern above:

const route = {
  path: "private",
  getChildRoutes: (location, cb) => {
    if (!authorized) {
      cb({ path: '*', status: 401, component: Unauthorized })
      return
    }

    cb(privateRoutes);
  }
}

@timdorr
Copy link
Member

timdorr commented Mar 22, 2016

That feels a lot more like a hack than something officially supported.

@taion
Copy link
Contributor Author

taion commented Mar 28, 2016

On second thought, I think @ryanflorence's approach here is best.

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

No branches or pull requests

3 participants