Skip to content

Warn on using Link outside of a Router #3889

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

Closed
Connorelsea opened this issue Sep 17, 2016 · 15 comments
Closed

Warn on using Link outside of a Router #3889

Connorelsea opened this issue Sep 17, 2016 · 15 comments

Comments

@Connorelsea
Copy link

Connorelsea commented Sep 17, 2016

I am unsure if I am doing anything wrong here and how to fix it.

Edit: Solved, check first comment.

Version

4.0.0-2

Steps to reproduce

Using create-react-app (with Babel and Webpack), create a Link by doing <Link to="/example" >Example</Link>

image

Expected Behavior

Should create a Link

Actual Behavior

Produces the following error:

Link.js:116 Uncaught TypeError: Cannot read property 'pathname' of undefined

image

The transpiled List file (See stack trace for error lines): https://gist.github.com/Connorelsea/66c14f7dfaa81b2150898663550fc2a0

When the transpiled code attempts to find the location, they are both undefined.

location || this.context.location becomes undefined || undefined

You can see the values of the variables of the transpiled code through Chrome dev tools in the following screenshot next to each variable.

image

@Connorelsea
Copy link
Author

Solved: The Link has to be rendered inside of a Router. Can this be explained better here and/or in the docs? I am starting to see the inner-workings by reading the code, but it would really be great to have a more clear stance in the docs about the inner-workings of this specifically.

@Connorelsea
Copy link
Author

Would it be easy to throw a more human readable error when a Link or Match or something is used outside of a router? I think I had the same problem earlier trying to use a Match outside of a Router parent. It is not immediately clear to a beginner why this is the case and a cryptic error led me down a more complicated debug path

@timdorr
Copy link
Member

timdorr commented Sep 17, 2016

We allow Link outside of Router on 2.x/3.0. We should probably do the same here and just create an a tag with warnings.

@timdorr timdorr changed the title Using webpack and babel, List in v4 says location/pathname are undefined Warn on using Link outside of a Router Sep 17, 2016
@timdorr timdorr added this to the v4.0.0 milestone Sep 17, 2016
@timdorr
Copy link
Member

timdorr commented Sep 17, 2016

Note, that we did this on 2.x in #3572

@Connorelsea
Copy link
Author

@timdorr Seems like the fix in 2.x was only adding an invariant. Yet the Link in 4.x breaks when used outside of the Router (because of the location). Should we fix the breaking and add the invariant, or let it continue to break, just adding an invariant?

@timdorr
Copy link
Member

timdorr commented Sep 17, 2016

We also have a guard to let folks render the a tag without a router instance.

@Connorelsea
Copy link
Author

Connorelsea commented Sep 17, 2016

@timdorr Ah, cool. Can I try to implement this in a PR?

@timdorr
Copy link
Member

timdorr commented Sep 17, 2016

Go nuts! It would be appreciated.

@Connorelsea
Copy link
Author

I'll try, but no promises. This would be my first open source PR. Thanks!

@Connorelsea
Copy link
Author

@timdorr Here, does it simply not return anything if it is not inside of a Router? Just so people can test props and stuff in unit tests? https://github.com/ReactTraining/react-router/blob/master/modules/Link.js#L114

@timdorr
Copy link
Member

timdorr commented Sep 17, 2016

Return statement is below that block. It just doesn't build up an extra props is all.

@Connorelsea
Copy link
Author

Ah. Sorry, was reading wrong.

@Connorelsea
Copy link
Author

@timdorr I think I've got a good start on it here. Please let me know what to change or what is OK when you've got a chance. Thanks!

@pshrmn
Copy link
Contributor

pshrmn commented Oct 23, 2016

With #3986 this is closer to working, but it fails in the handleTransition call which still requires a router.

There are two possible solutions:

  1. Add a check for the existence of this.context.router in handleClick. If this fails, then event.preventDefault() won't be called and the link will navigate naturally.
  2. Add a check for the existence of this.context.router in handleTransition. If this fails, use window.location to set the location to the this.props.to value.

Option 1 seems better to me.

Without access to the router, the <Link>'s to prop will need to be a string, which should probably be documented.

Active checks still work provided the user passes a location prop to the <Link>.

ryanflorence added a commit that referenced this issue Oct 26, 2016
we used to allow this to make unit tests easier, but now all you have
to do is wrap your component in a `<MemoryHistory>` and be on your way

closes #3889
@ryanflorence
Copy link
Member

98fe714

@remix-run remix-run deleted a comment from iqdev-ca Feb 8, 2018
@remix-run remix-run locked and limited conversation to collaborators Feb 8, 2018
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

4 participants