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

Make Link and NavLink components accept "to" property as a function #5368

Merged
merged 1 commit into from Jun 12, 2019
Merged

Make Link and NavLink components accept "to" property as a function #5368

merged 1 commit into from Jun 12, 2019

Conversation

smashercosmo
Copy link
Contributor

@smashercosmo smashercosmo commented Jul 23, 2017

closes #5331

Copy link
Contributor

@pshrmn pshrmn left a 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.

packages/react-router-dom/modules/Link.js Outdated Show resolved Hide resolved
packages/react-router-dom/modules/NavLink.js Outdated Show resolved Hide resolved
packages/react-router-dom/modules/__tests__/Link-test.js Outdated Show resolved Hide resolved
@smashercosmo
Copy link
Contributor Author

ok, it's been a while...)

  1. Almost all requested changes are addressed, except 'resolveToLocation' naming
  2. Documentation added
  3. More tests added
  4. Function passed to 'to' property can now return both objects and strings

please rereview @pshrmn @timdorr

@smashercosmo
Copy link
Contributor Author

There is one test failing in NavLink tests. I did some research and found out that test fails when withRouter is imported like this

import withRouter from "../withRouter";

changing this line to

import withRouter from "../../../react-router/modules/withRouter";

make test pass. I have no idea why ((

@timdorr
Copy link
Member

timdorr commented Mar 5, 2018

You have to run npm run build in the root first.

@smashercosmo
Copy link
Contributor Author

Did this. Still doesn't work. Test fails.

@pshrmn
Copy link
Contributor

pshrmn commented Mar 5, 2018

This doesn't work?

import withRouter from "react-router/withRouter";

It is used two lines above to import the <MemoryRouter>.

I generally run npx lerna bootstrap in the root to ensure packages are properly symlinked.

@smashercosmo
Copy link
Contributor Author

smashercosmo commented Mar 5, 2018

Didn't work with

import withRouter from "react-router/withRouter";

but npx lerna bootstrap did the trick) thx

@smashercosmo
Copy link
Contributor Author

Still don't know why travis is failing

@timdorr
Copy link
Member

timdorr commented Mar 5, 2018

Looks like Rollup is shitting the bed? Not your fault in that case. I'll try to take a look shortly.

@pshrmn
Copy link
Contributor

pshrmn commented Mar 5, 2018

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.

@timdorr
Copy link
Member

timdorr commented Mar 5, 2018

I just nuked all caches and restarted the build. We shall see...

@smashercosmo
Copy link
Contributor Author

Any response guys?) (no pressure, just pinging)

Copy link
Contributor

@pshrmn pshrmn left a 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.

@smashercosmo
Copy link
Contributor Author

@pshrmn did you come to any decision?) In V3 resolveToLocation function was also duplicated

@timdorr
Copy link
Member

timdorr commented Mar 15, 2018

I know it's a little bit of work, but moving those to a locationUtils.js module would be good.

@smashercosmo
Copy link
Contributor Author

@timdorr no problem. should it be put in react-router-dom package?

@pshrmn
Copy link
Contributor

pshrmn commented Mar 15, 2018

@smashercosmo I would just keep it in the react-router-dom package.

For parity this functionality should probably be added to react-router-native too. I'm not really sure where dev with that is going, but this would be a simple change. This doesn't necessarily need to be done with this PR, but feel free to go for it if you'd like.

(Even with adding this to r-r-n, I don't think it makes sense to include the location utility functions in core react-router)

@smashercosmo
Copy link
Contributor Author

done

@smashercosmo
Copy link
Contributor Author

Not quite sure if locationUtils need any tests or documentation

@timdorr timdorr force-pushed the master branch 4 times, most recently from 8f33936 to 1fb8696 Compare April 26, 2018 20:30
@smashercosmo
Copy link
Contributor Author

Hey, @mjackson @pshrmn @timdorr , are you still considering merging this PR? If yes, can it go into 4.4.0, if I fix all the conflicts?

@mjackson mjackson added this to the 4.4 milestone Nov 1, 2018
@smashercosmo
Copy link
Contributor Author

I won't stop poking you, guys :) @StringEpsilon @timdorr @pshrmn @mjackson

@timdorr
Copy link
Member

timdorr commented Jun 12, 2019

Fuck it; merge it. Let's try to get to a 5.1 release soon!

@timdorr timdorr merged commit bb802cd into remix-run:master Jun 12, 2019
@havgry
Copy link

havgry commented Jun 22, 2019

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 :)

@jancama2
Copy link

+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.

@croraf
Copy link

croraf commented Aug 15, 2019

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?

@smashercosmo
Copy link
Contributor Author

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?

@mjackson mjackson mentioned this pull request Aug 26, 2019
23 tasks
@mjackson
Copy link
Member

This was shipped today in 5.1.0. Sorry for the wait 😕

@lock lock bot locked as resolved and limited conversation to collaborators Nov 24, 2019
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.

Feature request: 'to' property as a function in Link component
10 participants