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

add <NotFoundRoute/> #140

Closed
mjackson opened this issue Jul 29, 2014 · 23 comments
Closed

add <NotFoundRoute/> #140

mjackson opened this issue Jul 29, 2014 · 23 comments
Labels
Milestone

Comments

@mjackson
Copy link
Member

In #112 we discussed adding a defaultHandler prop to Routes that would be responsible for handling situations where no routes in the tree match. Let's follow up on that work here.

@mjackson
Copy link
Member Author

In the discussion in #142 it is apparent that @rpflorence wants a way to nest his NotFound handler inside some other UI, which <Routes defaultHandler> doesn't give you.

Instead, let's extend the matching behavior of Routes without a name and path. Currently, if these routes have children that match, they also match. Otherwise, they don't.

The change will be to allow a Route with no children and no name/path to match as well. There is exactly one use case for such a route in a <Routes> config, so we need to throw if the user has more than one in the same tree. But at least this lets the user put their NotFound inside some other UI.

@mjackson mjackson changed the title Add defaultHandler to Routes Make Route with no children and no name/path match Jul 30, 2014
@ryanflorence
Copy link
Member

Renamed as solution discussed in #142

@ryanflorence ryanflorence changed the title Make Route with no children and no name/path match add <NotFoundRoute/> Jul 30, 2014
@ryanflorence
Copy link
Member

  • There should only be one <NotFoundRoute/>, throw if we find another
  • Can be nested like any other route, (though, probably, only once or not at all in practice)
  • Only takes a handler prop

@justinbelcher
Copy link

+1

@mtscout6
Copy link
Contributor

Ideally I would like to see more than one NotFoundRoute handler. For example:

<Routes>
  <Routes handler={Docs} initialPath='/docs'>
    ...
    <NotFoundRoute  handler={DocNotFound} />
  </Routes>
  <Routes handler={App}>
    ...
    <NotFoundRoute  handler={AppNotFound} />
  </Routes>
</Routes>

@mjackson
Copy link
Member Author

@mtscout6 Nested <Routes> are not supported.

@bripkens
Copy link
Contributor

bripkens commented Aug 3, 2014

+1

@ryanflorence
Copy link
Member

@mtscout6 NotFound means the url doesn't match any configured route at all, so there's no way to know which one you want to render when a route is not found, unless you've got some idea about what that initialPath means (maybe child routes inherit it?). You could accomplish this already:

<Routes>
  <Route handler={Docs}>
    ...
    <Route path="docs*" handler={DocNotFound} />
  </Route>
  <Route handler={App}>
    ...
    <Route path="*" handler={AppNotFound} />
  </Route>
</Routes>

Maybe there's a case to add path matching to <NotFoundRoute/> to accomplish similar behavior (render the one that matches) when #142 changes the meaning of *

@mtscout6
Copy link
Contributor

mtscout6 commented Aug 7, 2014

What I was primarily trying to get at, and maybe this is part of a separate issue in that being explicit with the url all the time is getting exhausting. Take for example what I've got going in a portion of our app so far: (This is just and small piece of a larger app, and we will be adding at least 30+ more routes under the CustomerRecord, many of which will be nested under these sub-urls)

<Route path='/customers/:customerId' handler={CustomerRecord}>
  <Route name='customer' path='/customers/:customerId' handler={CustomerRecordRedirect} />
  <Route name='familyMember' path='/customers/:customerId/family-member/:familyMember' handler={FamilyMemberRecord} />
  <Route name='quoting' path='/customers/:customerId/quoting' handler={CustomerQuoting} />
  <Route name='cart' path='/customers/:customerId/cart' handler={CustomerCart} />
  <Route name='applicationsAndPolicies' path='/customers/:customerId/applications-and-policies' handler={TasksAndAppointments} />
  <Route name='tasksAndAppointments' path='/customers/:customerId/tasks-and-appointments' handler={TasksAndAppointments} />
</Route>

In this example the root's path of '/customers/:customerId' Is getting repeated everywhere. The CustomerRecord handler needs that :customerId parameter. And we have no intentions to move any of those sub-routes out of the customer record.

So, I've found the customer it does exist, but the rest of the url does not, then I don't want to redirect them to an application 404. So, I was envisioning sub application 404s.

@ryanflorence
Copy link
Member

In this example the root's path of '/customers/:customerId' Is getting repeated everywhere.

We've talked about having /foo indicate to use an absolute path, and foo to inherit from the parent. I just use string concat:

CUSTOMER+'/family-member/:familyMember'`
CUSTOMER+'/quoting'

@mtscout6
Copy link
Contributor

mtscout6 commented Aug 7, 2014

I feel that that is an intermediary step, because now you have replaced '/customers/:customerId' with CUSTOMER+. As we go about nesting a route under /customers/:customerId/cart for example we would end up with

CUSTOMER = '/customers/:customerId';
CART = CUSTOMER + '/cart';
...
<Route name='cart-confirm' path={CART+'/confirmation'} />

Which is requiring a large amount of constants defined at the top when absolute vs relative support would do the same thing. Although that feels like a concern to address in #142.

What I'm getting at on this issue is that support for a <NotFound handler={Something} /> nested within the customer record for any urls after /customer/:customerId in this case would be outstanding.

@ryanflorence
Copy link
Member

can you provide some example urls "after /customer/:customerId" that would trigger the <NotFound/> route?

@mtscout6
Copy link
Contributor

mtscout6 commented Aug 7, 2014

/customer/:customerId/the-user-typed-something-in-the-url-bar

@ryanflorence
Copy link
Member

<Route path="/customer/:customerId/*" handler={NotFoundCustomer}/>

^ works today

@mtscout6
Copy link
Contributor

mtscout6 commented Aug 7, 2014

I know that would do, I'll drop it.

@mtscout6
Copy link
Contributor

mtscout6 commented Aug 7, 2014

Although, if you do the absolute vs relative path, now I'd have to use an absolute path in a sea of relative paths usage, assuming you adopt that approach.

@ryanflorence
Copy link
Member

its a valid use-case I want to support because we support it today. We need more powerful path matching and that might mean changing the meaning of *, so I want to make sure we can still do everything in the future that we can today.

@mjackson
Copy link
Member Author

The requirements here have changed because of the work done in #200.

Once we merge that, any <Route> with no name/path/children is considered a "default route" for its parent route, which functions like Ember's "index" routes. That solves part of the "not found" use case, but not the whole thing.

From this point forward, my suggestion for <NotFoundRoute> would be to make it work kind of like <DefaultRoute> inasmuch as

  1. you can only have one per <Route>,
  2. it automatically inherits the path of its parent, and
  3. it is order independent.

The key difference is that <NotFoundRoute> only uses the beginning of its path to match, instead of the entire thing. This would let you do stuff like:

<Routes>

  <!-- Matches /* after trying all other siblings -->
  <NotFoundRoute handler={NotFound}/>

  <!-- Matches / (purely UI) -->
  <Route handler={App}>

    <Route name="user" path="/users/:id" handler={User}>

      <!-- Matches /users/5 -->
      <DefaultRoute handler={Dashboard}/>

      <!-- Matches /users/5/* after trying all other siblings -->
      <NotFoundRoute handler={UserNotFound}/>

      <!-- Matches /users/5/news -->
      <Route path="/users/:id/news" handler={News}/>

    </Route>
  </Route>

</Routes>

@ryanflorence
Copy link
Member

yes, this is exactly how I imagined it working when we had both DefaultRoute and NotFoundRoute I must have had this example somewhere else, so many inter-related issues right now :P

@ryanflorence
Copy link
Member

Ah, here it is, the doozy of a post: #142 (comment)

@mjackson
Copy link
Member Author

so many inter-related issues right now

Seriously. Trying to keep them all straight in my head :) Would definitely help if we try and keep the scope of each one as small as possible and open new issues for tangents.

@ryanflorence
Copy link
Member

I collected the path stuff a while ago into a milestone, btw

@mjackson
Copy link
Member Author

I'm gonna try and get #142 resolved before this. Feels like this one depends on that.

@mjackson mjackson mentioned this issue Aug 26, 2014
mjackson added a commit that referenced this issue Aug 27, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Jan 25, 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

5 participants