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

Add support for React.forwardRef and React.createRef #278

Merged
merged 1 commit into from
Nov 20, 2018

Conversation

Andarist
Copy link
Contributor

No description provided.

@trotzig
Copy link
Collaborator

trotzig commented Nov 18, 2018

Hi @Andarist, and thanks for the contribution! This looks good to me, but I'm a little behind on the latest React features. Can you explain this change a little better? Preferably in the commit message so that future devs can understand why the change was made. More specifically, it would help me if you could explain if this is a breaking change or not (semver major vs semver minor/patch).

@Andarist
Copy link
Contributor Author

I've added a short explanation to the commit message:

Components wrapped with React.forwardRef will now correctly have their refs passed to react-waypoint as they don't recognize custom innerRef prop, but should know what to do with received ref (that is - pass it to underlaying DOM element).

Also the support for React.createRef has been added - which is a new React API, an alternative to callback refs. It returns an object with shape of { current: null }, so the ref can and should be set on the current property (it's done by React internally when passingit as ref prop to an element).

I can confirm that this change is backwards compatible. In the future (when doing a major release and dropping support for older React versions) you should dropping the innerRef convention entirely, always pass ref to the child component and enforce on people usage of React.forwardRef if they want a composite component as a child (ideally - you'd also provide a dev only warning when you detect non-forwardRef/DOM child). You can read slightly more about other libraries adopting this strategy here

@trotzig
Copy link
Collaborator

trotzig commented Nov 19, 2018

Thanks! The only thing I need before merging this PR is an update to the Readme. Will you take care of that?

@Andarist
Copy link
Contributor Author

I'll try to adjust it some time later.

Components wrapped with `React.forwardRef` will now correctly have their refs passed to `react-waypoint` as they don't recognize custom `innerRef` prop, but should know what to do with received `ref` (that is - pass it to underlaying DOM element).

Also the support for `React.createRef` has been added - which is a new React API, an alternative to callback refs. It returns an object with shape of `{ current: null }`, so the ref can and should be set on the `current` property (it's done by React internally when passingit as ref prop to an element).
@Andarist
Copy link
Contributor Author

I've adjusted the PR with README change.

@trotzig
Copy link
Collaborator

trotzig commented Nov 20, 2018

Great! Thanks for the quick turnaround.

@trotzig trotzig merged commit 5a72874 into civiccc:master Nov 20, 2018
@trotzig
Copy link
Collaborator

trotzig commented Nov 20, 2018

Released in 8.1.0

@Andarist Andarist deleted the support-forward-ref branch November 20, 2018 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants