-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Make Link and NavLink components accept "to" property as a function #5368
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me. I added a few comments on things that don't necessarily have to be changed (so I won't put a block on this), but should probably at least be considered.
fb3a586
to
f9481bb
Compare
There is one test failing in NavLink tests. I did some research and found out that test fails when withRouter is imported like this
changing this line to
make test pass. I have no idea why (( |
You have to run |
Did this. Still doesn't work. Test fails. |
This doesn't work? import withRouter from "react-router/withRouter"; It is used two lines above to import the I generally run |
Didn't work with import withRouter from "react-router/withRouter"; but |
Still don't know why travis is failing |
Looks like Rollup is shitting the bed? Not your fault in that case. I'll try to take a look shortly. |
I just updated another PR so it would run on Travis and there is definitely an issue there. Maybe it's just a bad cache? I believe that that was updated with the website auto-deploy. |
I just nuked all caches and restarted the build. We shall see... |
Any response guys?) (no pressure, just pinging) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, both Link.js
and NavLink.js
duplicate the resolveToLocation
and normalizeToLocation
functions. Part of me thinks that they should be moved to a locationUtils.js
and part thinks that they should just be inlined.
@pshrmn did you come to any decision?) In V3 resolveToLocation function was also duplicated |
I know it's a little bit of work, but moving those to a |
@timdorr no problem. should it be put in react-router-dom package? |
@smashercosmo I would just keep it in the For parity this functionality should probably be added to (Even with adding this to |
done |
Not quite sure if locationUtils need any tests or documentation |
8f33936
to
1fb8696
Compare
I won't stop poking you, guys :) @StringEpsilon @timdorr @pshrmn @mjackson |
Fuck it; merge it. Let's try to get to a 5.1 release soon! |
I just wanted to point out that I also wasted time because of the discrepancy between the documentation and the source code. If 5.1 isn't being released anytime soon, I'd suggest updating the documentation :) |
+1 on removing it from the docs since it is pretty misleading, took me a while before realizing that the issue is not on my side. |
Is the function intended to do a sideefects in it, like when the Link is clicked to make a logout action and then return the next location? |
Hey @timdorr , maybe it's easier to just make a release, while @mjackson and @ryanflorence are figuring out the future path for @reach/router and react-router? There is a bunch of fixes and new features in master. What is stopping us from releasing them? |
This was shipped today in 5.1.0. Sorry for the wait 😕 |
closes #5331