Skip to content
This repository has been archived by the owner on Dec 15, 2018. It is now read-only.

route list must be complete #39

Closed
dpwrussell opened this issue Aug 24, 2016 · 7 comments
Closed

route list must be complete #39

dpwrussell opened this issue Aug 24, 2016 · 7 comments

Comments

@dpwrussell
Copy link

dpwrussell commented Aug 24, 2016

It was not clear to me that the list of routes passed to createStoreWithRouter must be complete. I think this led me astray:

// Arbitrary data to add to the state tree when a route is
// matched and dispatched. Useful for page titles and other
// route-specific data.

I figured I didn't need that to get started. Perhaps it ultimately wont be required? Right now, trying to use a link that isn't in there leads to:

fragment.js:27 Uncaught TypeError: Cannot read property 'route' of null

It made sense to me that there could be fragments which didn't correspond to preconfigured routes.

Split out from #35

@jondelga
Copy link

I came across this error as well.

@tptee
Copy link
Contributor

tptee commented Aug 25, 2016

I'm getting a PR ready to make the routes object optional 👍 I'm realizing that it's an advanced feature that isn't necessary for the rest of the library to function. I'll break route matching into a separate "Advanced" section so that it doesn't distract from the examples in the beginning.

@tptee
Copy link
Contributor

tptee commented Aug 25, 2016

^ Disregard that, I just remembered why preconfigured routes must be provided. Without a defined order for routes, <Fragment> isn't able to distinguish between certain routes with named parameters.

If you have a <Fragment> that matches /home, and another <Fragment> that matches /:things, both fragments will always appear on /home. The route /home matches both /home and /:things.

You need a defined order to ensure that concrete routes like /home take precedence over parameterized routes like /:things.

I think we'll keep requiring the route object unless we can find a solution to that in the future (I can't think of any way to make it work, though). I've updated the docs to clarify the purpose of configured routes and to mark them as required.

@dpwrussell
Copy link
Author

Thanks for the clarification. One follow up though:

This make sense as the desire would always be to match /home before /:things (I don't think react router supports this at all!). That rings true to me as long as not every Fragment must match a route? You can easily imagine that sometimes there is a desire to match based on location criteria without a specific route being matched (or perhaps there is and that is just part of the location being matched).

An example might be a chat component that exists near the root of the component hierarchy, but it should get displayed whenever the route component is ...?chat=true or even .../chat. Then it can display correctly without having to be explicitly added as a child route (in the latter case) to every other route configuration. Does that make sense?

@jondelga
Copy link

I think that's good start, but it can be not too obvious to the user why something is failing. If there's a typo on one of the routes in the route object, this fragment.js:27 Uncaught TypeError: Cannot read property 'route' of null will happen because matchRoute(location.pathname) will return null and fragment.js will still try to get route from it.

@tptee
Copy link
Contributor

tptee commented Aug 26, 2016

Two PRs in progress to address this:
#45 adds useful error messages when routes are empty or malformed.
#43 fixes the missing null check @kriuz described above.

@dpwrussell, you might want to look at the withConditions prop for <Fragment>, which doesn't require a route definition at all:

<Fragment withConditions={location => location.query.superuser}>
  <p>Superusers see this on all routes!</p>
</Fragment>

@tptee tptee removed the docs label Aug 26, 2016
@tptee tptee reopened this Aug 26, 2016
@tptee
Copy link
Contributor

tptee commented Aug 26, 2016

Merged #43 and #45, closing!

@tptee tptee closed this as completed Aug 26, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants