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

Breaking change between version 5.0.1 and 5.1.0 #6934

Closed
kennethlarkin opened this issue Sep 26, 2019 · 10 comments
Closed

Breaking change between version 5.0.1 and 5.1.0 #6934

kennethlarkin opened this issue Sep 26, 2019 · 10 comments

Comments

@kennethlarkin
Copy link

kennethlarkin commented Sep 26, 2019

Version

5.1.0

Test Case

upgrading a site between 5.0.1 and 5.1.0 breaks the site.

Steps to reproduce

add a <link> inside a <browserRouter>

let BrowserRouter = ReactRouterDOM.BrowserRouter;
let Link = ReactRouterDOM.Link;

adding a to= doesnt make a difference

return (
<BrowserRouter>
<Link className="nav-link" >Home</Link>

Expected Behavior

link rendered

Actual Behavior

backend.js:6 Warning: Stateless function components cannot be given refs. Attempts to access this ref will fail.

Check the render method of e.
in Unknown (created by e)
in e
in Unknown (created by AppLayout)
in t (created by t)
in t (created by t)
in t (created by AppLayout)
in AppLayout

@support
Copy link

support bot commented Sep 26, 2019

👋 @kennethlarkin, we use the issue tracker exclusively for bug reports and feature requests. However, this issue appears to be a support request. For usage questions, please use Stack Overflow or Reactiflux where there are a lot more people ready to help you out.
Please feel free to clarify your issue if you think it was closed prematurely.

@support support bot closed this as completed Sep 26, 2019
@kennethlarkin
Copy link
Author

Tim, thanks for replying. Not looking for Support I am looking to report a bug. If the code was working on major version 5 it should continue to work on all major version 5 sub versions.

I have included steps to reproduce the bug above and the expected behavior. i have looked at the release notes and this should not have been affected.

@timdorr timdorr reopened this Sep 26, 2019
@support support bot removed the support label Sep 26, 2019
@timdorr
Copy link
Member

timdorr commented Sep 26, 2019

Those steps aren't sufficient, as they don't include detailed enough data. Can you reproduce this on codesandbox.io? At the very least, can you produce a stacktrace that isn't minimized.

@kennethlarkin
Copy link
Author

There is already an example on there:
https://codesandbox.io/s/a-simple-react-router-v4tutorial-gtg3e

Change the react-router-dom version to latest and it breaks

@StringEpsilon
Copy link
Contributor

StringEpsilon commented Sep 26, 2019

Seems to affect only react <= 16.2.x

With RR 5.1 and React 16.3, it works.

Root cause is most likely
facebook/react#4936
in conjunction with @ryanflorence' patch here:

b5528ed

Given that we still "support" react 15 (and 16.0 - 16.2), this sure is a breakage.

@timdorr
Copy link
Member

timdorr commented Sep 26, 2019

So, a couple of things there:

  • Creating a polyfill there isn't the right approach. It should be a conditional that returns either the component or forwarded component if the API exists.
  • On React Redux, We found there was a notable performance degradation when using forwardRef. It would be ideal if it was optional and opt-in.

@mjackson
Copy link
Member

I'm confused as to how the forwardRef polyfill in that patch changes anything at all in React 15. It's just the identity function. So before the patch, we had:

function Link(props) {
  // ...
}

export { Link }

And after the patch we have:

let forwardRef = C => C;

const Link = forwardRef((props, ref) => {
  // ...
})

export { Link }

All React 15 sees is a function that takes some props.

Is this line the one that causes the breakage??

@timdorr
Copy link
Member

timdorr commented Sep 27, 2019

Yep, that's what's doing it.

@mjackson
Copy link
Member

Ah, I see. The <LinkAnchor> component is a function component and it's getting a ref. Will fix.

mjackson added a commit that referenced this issue Sep 27, 2019
If we can't use forwardRef, we shouldn't ever pass a ref to a
functional component. Use innerRef workaround instead.

Fixes #6934
@mjackson mjackson mentioned this issue Sep 27, 2019
23 tasks
@mjackson
Copy link
Member

mjackson commented Sep 27, 2019

This was fixed and released in 5.1.1 https://github.com/ReactTraining/react-router/releases/tag/v5.1.1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants