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

Explicit error on <Route> outside <Router> #4939

Merged
merged 9 commits into from
Jul 14, 2017
Merged

Explicit error on <Route> outside <Router> #4939

merged 9 commits into from
Jul 14, 2017

Conversation

eXon
Copy link
Contributor

@eXon eXon commented Apr 9, 2017

When using outside a valid , the message is not that explicit. It throws:

Cannot read property 'route' of undefined

Even though it's not that bad, I propose we make it even more explicit. Can't be bad to know where the error is coming from. If you made a typo when importing the router, it's not obvious otherwise.

This will throw an error rather than a warning because the library code would crash otherwise.

eXon added 3 commits April 9, 2017 13:00
When using <Route> outside a valid <Router>, the message is not that explicit. It throws:

`Cannot read property 'route' of undefined`

Even though it's not that bad, I propose we make it even more explicit. Can't be bad to know where the error is coming from. If you made a typo when importing the router, it's not obvious otherwise.

This will throw an error rather than a warning because the library code would crash otherwise.
@timdorr
Copy link
Member

timdorr commented Apr 9, 2017

Yeah, I'd rather it does this to be honest. 👍 from me.

if (computedMatch)
return computedMatch // <Switch> already computed the match for us

if (!router) {
throw new Error('You should not use <Route> or withRoute() outside a valid <Router>')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

withRouter not withRoute.

Also, shouldn't this be done with warning or invariant?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, yes, I would do this via an invariant. We can optimize those calls out for production builds that way.

@agundermann
Copy link
Contributor

👍

Might be worth doing the same for the other components (<Link> etc.).

@eXon
Copy link
Contributor Author

eXon commented Apr 11, 2017

I've switch the throw by invariant and added the check to <Route>, <Link>, <Switch>, <Redirect>, <Prompt>

The only components that require the router that I haven't added it are within the react-native project. I don't have an environment setup and I'm not sure if I can use the invariant the same way.

@eXon
Copy link
Contributor Author

eXon commented Apr 29, 2017

Any update on this PR?

@mjackson
Copy link
Member

This PR looks great, thanks @eXon. Any chance you can fix the conflict and rebase? I'll be happy to merge after you do.

@timdorr
Copy link
Member

timdorr commented Jul 14, 2017

It was a minor conflict. Want to merge it in?

@mjackson
Copy link
Member

Thanks, @timdorr. Sure, go for it. I've got some Jest stuff I wanna push by EOD, so it may conflict a little. But I'll handle that.

@timdorr timdorr merged commit fabcecc into remix-run:master Jul 14, 2017
@timdorr
Copy link
Member

timdorr commented Jul 14, 2017

Let me know when that's done. We need to re-build and publish a patch release for 4.1.1, as the UMD build is faulty due to a prop-types bug that existed when it was published. Might as well roll that all together into a 4.2.0 release.

@eXon
Copy link
Contributor Author

eXon commented Jul 14, 2017

Thanks for your awesome work guys! 👍

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.

5 participants