fix: enable ref forwarding with forwardRef #9892
Conversation
Note: I think we should minimally make this more than a patch release per [React docs](https://reactjs.org/docs/forwarding-refs.html#note-for-component-library-maintainers) This will make the Link component work more like many would expect (innerRef is confusing!), but it does come at the cost of requiring React 16.3. I think it's a reasonable trade off, however.
| @@ -97,7 +97,6 @@ class GatsbyLink extends React.Component { | |||
| /* eslint-disable no-unused-vars */ | |||
| activeClassName: $activeClassName, | |||
| activeStyle: $activeStyle, | |||
| ref: $ref, | |||
There was a problem hiding this comment.
@LekoArts this fixes the bug re: accessing ref.
This is tricky - if this is breaking change then we would need bump major version both in |
|
@pieh I'll check this out locally, but I think this is a valuable change we should get in. I don't think it is actually a breaking change if we already require >= 16.3 in Gatsby. |
|
Few things:
|
|
Gatsby v2 was released after react 16.4.2 was out so that's the minimum version we support. We could probably get away with bumping the minimum required react version in the future with sufficient reason. |
|
@KyleAMathews yeah - I'm thinking once some of the newer, useful stuff launches, e.g. Suspense with SSR, Hooks, etc. that could be a good time to bump the peerDep. In effect--if it makes the code we maintain smaller, cleaner, etc. that's a good time to make that change. What do you think? |
The currently used version of "Gatsby Link" (1) uses the `innerRef` prop to allow `ref` access to the underlying DOM element (necessary for animations). In order to wrap animated components the `ref` is necessary and is therefore now forwarded. The legacy behavior of "Gatsby Link" is about to change in the future to the `React.forwardRef` API. See gatsbyjs/gatsby#9892 for more details. References: (1) https://www.gatsbyjs.org/docs/gatsby-link Associated epic: GH-69 GH-64
|
Yeah, the next thing that should really impact us is suspense as we could replace a lot of our preloading/prefetching/page transition code with it. Hooks are great but I don't think there's anywhere we'd use them in the Gatsby runtime? |
|
I think when I tested it back then everything worked 🤔 |
|
Note: I think we should minimally make this more than a patch release per [React docs](https://reactjs.org/docs/forwarding-refs.html#note-for-component-library-maintainers) This will make the Link component work more like many would expect (innerRef is confusing!), but it does come at the cost of requiring React 16.3. I think it's a reasonable trade off, however. cc @LekoArts I validated this with a sample repo, but want to pull this down and make sure it fixes your issue, as well? <!-- Q. Which branch should I use for my pull request? A. Use `master` branch (probably). Q. Which branch if my change is a bug fix for Gatsby v1? A. In this case, you should use the `v1` branch Q. Which branch if I'm still not sure? A. Use `master` branch. Ask in the PR if you're not sure and a Gatsby maintainer will be happy to help :) Note: We will only accept bug fixes for Gatsby v1. New features should be added to Gatsby v2. Learn more about contributing: https://www.gatsbyjs.org/docs/how-to-contribute/ -->
Note: I think we should minimally make this more than a patch release per
React docs
This will make the Link component work more like many would expect
(innerRef is confusing!), but it does come at the cost of requiring
React 16.3. I think it's a reasonable trade off, however.
cc @LekoArts I validated this with a sample repo, but want to pull this down and make sure it fixes your issue, as well?