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

Fix <Link to="string"> #5489

Merged
merged 3 commits into from Oct 3, 2017
Merged

Fix <Link to="string"> #5489

merged 3 commits into from Oct 3, 2017

Conversation

pshrmn
Copy link
Contributor

@pshrmn pshrmn commented Sep 1, 2017

New implementation will parse the string to create an actual location object. This also means that a to string with no pathname will resolve using the current location.

The href value is also now stored in state, only recalculating it when the to prop changes.

Edit: If anyone is looking at this, but hasn't read through #5488, the reason for this PR is that when a user renders a <Link> whose string to does not include a pathname, the rendered anchor's href does not include a pathname (<Link to='?test=ing'>Test</Link> -> <a href='?test=ing'>Test</a>). This results in conflicts with the output of the <StaticRouter> since that explicitly makes hrefs absolute paths by enforcing that the string begins with a forward slash. Since React Router works with location's whose pathnames are absolute, I felt it was more appropriate for the <Link> to produce absolute URLs than it was for the <StaticRouter> to have a special use case.

@timdorr
Copy link
Member

timdorr commented Sep 1, 2017

Why not keep things inside the render? Does createLocation have some sort of side effect?

@pshrmn
Copy link
Contributor Author

pshrmn commented Sep 1, 2017

We could have the funcionality in render, but with string to props, that would mean that we re-parse the string every render. Using state also saves us from calling createHref every render. This isn't a huge performance win, but I'm not aware of any negative effects this would cause either.

New implementation will parse the string to create an actual location object. This also means that a to string with no pathname will resolve using the current location.

The "href" value is also now stored in state, only recalculating it when the "to" prop changes.
@mxstbr
Copy link
Contributor

mxstbr commented Sep 4, 2017

Would love to see this land! 😊

@mjackson
Copy link
Member

mjackson commented Sep 13, 2017

I'd definitely like to keep the create call inside render. If it's a perf problem we can use sCU, but I'd like to see some proof that perf is bad before we make that optimization.

@pshrmn
Copy link
Contributor Author

pshrmn commented Sep 13, 2017

I reverted to computing the href in render.

@timdorr
Copy link
Member

timdorr commented Oct 3, 2017

I think this is good too. In she goes...

@timdorr timdorr merged commit be6fb6d into remix-run:master Oct 3, 2017
@mxstbr
Copy link
Contributor

mxstbr commented Oct 4, 2017

YAS 🎉 Was this released?

@pshrmn pshrmn deleted the fix-link-to branch November 12, 2017 05:24
@remix-run remix-run deleted a comment from brendanmoore Jan 16, 2018
bors bot added a commit to mozilla/delivery-console that referenced this pull request Jun 6, 2018
179: Update dependency react-router-dom to v4.3.0 r=magopian a=renovate[bot]

This Pull Request updates dependency [react-router-dom](https://github.com/ReactTraining/react-router) from `v4.2.2` to `v4.3.0`



<details>
<summary>Release Notes</summary>

### [`v4.3.0`](https://github.com/ReactTraining/react-router/blob/master/CHANGELOG.md#v430httpsgithubcomReactTrainingreact-routercomparev420v430)
[Compare Source](remix-run/react-router@v4.3.0-rc.3...a27bc56)
> Jun 6, 2018

* Use the `pretty` option in generatePath ([#&#8203;6172] by @&#8203;sibelius)
* aria-current has incorrect value "true" ([#&#8203;6118] by @&#8203;brandonrninefive)
* Redirect with parameters ([#&#8203;5209] by @&#8203;dlindenkreuz)
* Fix with missing pathname: `<Link to="?foo=bar">` ([#&#8203;5489] by @&#8203;pshrmn)
* Escape NavLink path to allow special characters in path. ([#&#8203;5596] by @&#8203;esiegel)
* Expose `generatePath` ([#&#8203;5661] by @&#8203;rybon)
* Use named import of history module. ([#&#8203;5589] by @&#8203;RoboBurned)
* Hoist dependencies for smaller UMD builds ([#&#8203;5720] by @&#8203;pshrmn)
* Remove aria-current from navLink when inactive ([#&#8203;5508] by @&#8203;AlmeroSteyn)
* Add invariant for missing "to" property on `<Link>` ([#&#8203;5792] by @&#8203;selbekk)
* Use Prettier on the code ([e6f9017] by @&#8203;mjackson)
* Fix pathless route's match when parent is null ([#&#8203;5964] by @&#8203;pshrmn)
* Use history.createLocation in `<StaticRouter>` ([#&#8203;5722] by @&#8203;pshrmn)

[#&#8203;6172]: `remix-run/react-router#6172
[#&#8203;6118]: `remix-run/react-router#6118
[#&#8203;5209]: `remix-run/react-router#5209
[#&#8203;5489]: `remix-run/react-router#5489
[#&#8203;5596]: `remix-run/react-router#5596
[#&#8203;5661]: `remix-run/react-router#5661
[#&#8203;5589]: `remix-run/react-router#5589
[#&#8203;5720]: `remix-run/react-router#5720
[#&#8203;5508]: `remix-run/react-router#5508
[#&#8203;5792]: `remix-run/react-router#5792
[e6f9017]: remix-run/react-router@e6f9017
[#&#8203;5964]: `remix-run/react-router#5964
[#&#8203;5722]: `remix-run/react-router#5722

---

### [`v4.3.0-rc.3`](https://github.com/ReactTraining/react-router/releases/v4.3.0-rc.3)
[Compare Source](remix-run/react-router@v4.3.0-rc.2...v4.3.0-rc.3)
- Fix broken UMD builds.
- Add sideEffects: false for webpack tree shaking (#&#8203;6082 by @&#8203;taylorc93)

---

### [`v4.3.0-rc.2`](https://github.com/ReactTraining/react-router/releases/v4.3.0-rc.2)
[Compare Source](remix-run/react-router@v4.3.0-rc.1...v4.3.0-rc.2)
- Bump `hoist-non-react-statics` for React 16.3.
- Missing `generatePath` in `react-router-dom` package.

---

### [`v4.3.0-rc.1`](https://github.com/ReactTraining/react-router/releases/v4.3.0-rc.1)
[Compare Source](remix-run/react-router@v4.2.2...v4.3.0-rc.1)
> Mar 26, 2018

- Redirect with parameters ([#&#8203;5209] by @&#8203;dlindenkreuz)
- Fix with missing pathname: `<Link to="?foo=bar">` ([#&#8203;5489] by @&#8203;pshrmn)
- Escape NavLink path to allow special characters in path. ([#&#8203;5596] by @&#8203;esiegel)
- Expose `generatePath` ([#&#8203;5661] by @&#8203;rybon)
- Use named import of history module. ([#&#8203;5589] by @&#8203;RoboBurned)
- Hoist dependencies for smaller UMD builds ([#&#8203;5720] by @&#8203;pshrmn)
- Remove aria-current from navLink when inactive ([#&#8203;5508] by @&#8203;AlmeroSteyn)
- Add invariant for missing "to" property on `<Link>` ([#&#8203;5792] by @&#8203;selbekk)
- Use Prettier on the code ([e6f9017] by @&#8203;mjackson)
- Fix pathless route's match when parent is null ([#&#8203;5964] by @&#8203;pshrmn)
- Use history.createLocation in `<StaticRouter>` ([#&#8203;5722] by @&#8203;pshrmn)

[#&#8203;5209]: `remix-run/react-router#5209
[#&#8203;5489]: `remix-run/react-router#5489
[#&#8203;5596]: `remix-run/react-router#5596
[#&#8203;5661]: `remix-run/react-router#5661
[#&#8203;5589]: `remix-run/react-router#5589
[#&#8203;5720]: `remix-run/react-router#5720
[#&#8203;5508]: `remix-run/react-router#5508
[#&#8203;5792]: `remix-run/react-router#5792
[e6f9017]: remix-run/react-router@e6f9017
[#&#8203;5964]: `remix-run/react-router#5964
[#&#8203;5722]: `remix-run/react-router#5722

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
bors bot added a commit to mozilla/delivery-console that referenced this pull request Jun 6, 2018
178: Update dependency react-router to v4.3.0 r=magopian a=renovate[bot]

This Pull Request updates dependency [react-router](https://github.com/ReactTraining/react-router) from `v4.2.0` to `v4.3.0`



<details>
<summary>Release Notes</summary>

### [`v4.3.0`](https://github.com/ReactTraining/react-router/blob/master/CHANGELOG.md#v430httpsgithubcomReactTrainingreact-routercomparev420v430)
[Compare Source](remix-run/react-router@v4.3.0-rc.3...v4.3.0)
> Jun 6, 2018

* Use the `pretty` option in generatePath ([#&#8203;6172] by @&#8203;sibelius)
* aria-current has incorrect value "true" ([#&#8203;6118] by @&#8203;brandonrninefive)
* Redirect with parameters ([#&#8203;5209] by @&#8203;dlindenkreuz)
* Fix with missing pathname: `<Link to="?foo=bar">` ([#&#8203;5489] by @&#8203;pshrmn)
* Escape NavLink path to allow special characters in path. ([#&#8203;5596] by @&#8203;esiegel)
* Expose `generatePath` ([#&#8203;5661] by @&#8203;rybon)
* Use named import of history module. ([#&#8203;5589] by @&#8203;RoboBurned)
* Hoist dependencies for smaller UMD builds ([#&#8203;5720] by @&#8203;pshrmn)
* Remove aria-current from navLink when inactive ([#&#8203;5508] by @&#8203;AlmeroSteyn)
* Add invariant for missing "to" property on `<Link>` ([#&#8203;5792] by @&#8203;selbekk)
* Use Prettier on the code ([e6f9017] by @&#8203;mjackson)
* Fix pathless route's match when parent is null ([#&#8203;5964] by @&#8203;pshrmn)
* Use history.createLocation in `<StaticRouter>` ([#&#8203;5722] by @&#8203;pshrmn)

[#&#8203;6172]: `remix-run/react-router#6172
[#&#8203;6118]: `remix-run/react-router#6118
[#&#8203;5209]: `remix-run/react-router#5209
[#&#8203;5489]: `remix-run/react-router#5489
[#&#8203;5596]: `remix-run/react-router#5596
[#&#8203;5661]: `remix-run/react-router#5661
[#&#8203;5589]: `remix-run/react-router#5589
[#&#8203;5720]: `remix-run/react-router#5720
[#&#8203;5508]: `remix-run/react-router#5508
[#&#8203;5792]: `remix-run/react-router#5792
[e6f9017]: remix-run/react-router@e6f9017
[#&#8203;5964]: `remix-run/react-router#5964
[#&#8203;5722]: `remix-run/react-router#5722

---

### [`v4.3.0-rc.3`](https://github.com/ReactTraining/react-router/releases/v4.3.0-rc.3)
[Compare Source](remix-run/react-router@v4.3.0-rc.2...v4.3.0-rc.3)
- Fix broken UMD builds.
- Add sideEffects: false for webpack tree shaking (#&#8203;6082 by @&#8203;taylorc93)

---

### [`v4.3.0-rc.2`](https://github.com/ReactTraining/react-router/releases/v4.3.0-rc.2)
[Compare Source](remix-run/react-router@v4.3.0-rc.1...v4.3.0-rc.2)
- Bump `hoist-non-react-statics` for React 16.3.
- Missing `generatePath` in `react-router-dom` package.

---

### [`v4.3.0-rc.1`](https://github.com/ReactTraining/react-router/releases/v4.3.0-rc.1)
[Compare Source](remix-run/react-router@v4.2.0...v4.3.0-rc.1)
> Mar 26, 2018

- Redirect with parameters ([#&#8203;5209] by @&#8203;dlindenkreuz)
- Fix with missing pathname: `<Link to="?foo=bar">` ([#&#8203;5489] by @&#8203;pshrmn)
- Escape NavLink path to allow special characters in path. ([#&#8203;5596] by @&#8203;esiegel)
- Expose `generatePath` ([#&#8203;5661] by @&#8203;rybon)
- Use named import of history module. ([#&#8203;5589] by @&#8203;RoboBurned)
- Hoist dependencies for smaller UMD builds ([#&#8203;5720] by @&#8203;pshrmn)
- Remove aria-current from navLink when inactive ([#&#8203;5508] by @&#8203;AlmeroSteyn)
- Add invariant for missing "to" property on `<Link>` ([#&#8203;5792] by @&#8203;selbekk)
- Use Prettier on the code ([e6f9017] by @&#8203;mjackson)
- Fix pathless route's match when parent is null ([#&#8203;5964] by @&#8203;pshrmn)
- Use history.createLocation in `<StaticRouter>` ([#&#8203;5722] by @&#8203;pshrmn)

[#&#8203;5209]: `remix-run/react-router#5209
[#&#8203;5489]: `remix-run/react-router#5489
[#&#8203;5596]: `remix-run/react-router#5596
[#&#8203;5661]: `remix-run/react-router#5661
[#&#8203;5589]: `remix-run/react-router#5589
[#&#8203;5720]: `remix-run/react-router#5720
[#&#8203;5508]: `remix-run/react-router#5508
[#&#8203;5792]: `remix-run/react-router#5792
[e6f9017]: remix-run/react-router@e6f9017
[#&#8203;5964]: `remix-run/react-router#5964
[#&#8203;5722]: `remix-run/react-router#5722

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
@manjufy
Copy link

manjufy commented Jun 12, 2018

This change causing the href calculations to be
http://abc.com/http://abc.com/contact.html
After upgrading to never version, href calculation seems to be messed up

@pshrmn
Copy link
Contributor Author

pshrmn commented Jun 12, 2018

@manju16832003 Can you provide sample code (or ideally a sandbox) to reproduce the error?

@manjufy
Copy link

manjufy commented Jun 13, 2018

<Link to="http://abc.com/ContactList.html"></Link>
This is how I used it before and used to work, however, after upgrading it to new version, it broke :-).

@pshrmn
Copy link
Contributor Author

pshrmn commented Jun 13, 2018

@manju16832003 You should only be including the pathname (and optionally search/hash) for a location, not the scheme or domain.

<Link to="/ContactList.html">Contact List</Link>

@Android3000
Copy link

@pshrmn Interesting. We ran into the same thing as @manju16832003 too. I guess I never picked up in the documentation that Link couldn't handle external links (which up until this point, it could). Maybe this could be made more explicit in the documentation?

While obviously Link is intended for internal navigation, I'm sure there are other users who may be running into similar things too -- I could easily see a use case impacted by this in a breaking way, like say a re-usable component with a Link inside that sometimes may be provided an internal URL, and sometimes an external URL. Easy enough to fix if one is aware, but something I could see coming up as a surprise for some folks.

Either way, thanks for the updates!

@Skandar-Ln
Copy link

Suppose the location path is /task/list,I have a Link
<Link to="task/detail/xxx" target="_blank">detail</Link>
before this pr,click the link we get the new window of path task/detail/xxx,but after it,I get /task/list/task/detail/xxx,So, it's a bug。Did you think of it before?

@pshrmn
Copy link
Contributor Author

pshrmn commented Aug 9, 2018

@Skandar-Ln you are using a relative pathname (no leading slash), so the pathname is being appended to the current location's pathname.

@noahm
Copy link

noahm commented Sep 18, 2018

I just wanted to point out that like @manjufy, we were using the component more or less universally and this is a breaking change. I also agree with @Android3000: the release notes for 4.3.0 should definitely be clear about this change along with a note the general docs.

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.

None yet

8 participants