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 definition for React.forwardRef() #6510

Closed
wants to merge 1 commit into from

Conversation

motiz88
Copy link
Contributor

@motiz88 motiz88 commented Jun 23, 2018

Closes #6103. There is some context re: soundness in #6103 (comment).

The type of the render argument here diverges very slightly from that in React master at the moment, but see facebook/react#13100 for the reason why.

UPDATE: With facebook/react#13100 now merged, this is aligned with the React source code's own definition of forwardRef.

@AlmeroSteyn
Copy link

Hi!

Started introducing Flow into my React project last week. As we focus on accessibility a lot, there is a lot of focus management and therefore usage of Refs. So we immediately bumped into this one with forwardRef.

I understand how these projects go and therefore understand that everyone is busy, but could I ask if there is any idea when Flow will support the new 16.3 features fully?

Is there also some clear documentation/examples somewhere showing how to use Flow with React.createRef?

Thank you so much for building tools like this and maintaining it.

@motiz88
Copy link
Contributor Author

motiz88 commented Aug 12, 2018

Hey @AlmeroSteyn,

  1. I think [meta] Bringing Flow types in line with 16.3 APIs react#12553 gives a complete picture of React 16.3 features in Flow and that's probably the best place to discuss them as a whole. I wish I could tell you anything about timelines, but your guess is as good as mine I'm afraid.

  2. https://flow.org/en/docs/react/refs/ is probably due for an update documenting createRef (and forwardRef for that matter, when this is merged) since it only mentions ref functions at the moment. I'm not aware of any other source of documentation for this, but I'm sure a docs PR will be welcome 😉

@AlmeroSteyn
Copy link

@motiz88, thanks for the reponse.

Had to backtrack on using Flow as we use ref a lot for accessibility (focus control) and also could not find out how to properly type ...rest object spreads. So my code ended up in an any mess.

Thanks for the invite to participate with PR's, however other areas of focus are sapping my time currently. Not that it would help though, the problem is that I cannot get these types working at all LOL. Much less document them.

@motiz88
Copy link
Contributor Author

motiz88 commented Aug 15, 2018

@mrkev Any chance you could give this a look? I'm not overjoyed about the #6103 (comment) situation but I think it's the best we can get out of the current mechanics of Flow, and the drawbacks are outweighed by the benefits of providing some form of support for this React feature, 5 months now since its release.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

motiz88 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not at all familiar with the Flow codebase– so someone like @samwgoldman or @avikchaudhuri should have the final say here, but I think the forwardRef type looks right to me! 👍

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

motiz88 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

TrySound referenced this pull request Nov 27, 2018
Reviewed By: gabelevi, pakoito

Differential Revision: D13217025

fbshipit-source-id: af6f93077fb5b457b2e2fc516f583dc0fb4df776
@samwgoldman
Copy link
Member

This was reviewed internally a while back, but we never updated this PR. Sorry about that. The proposed types here are not quite correct and lead to missed errors when the ref pseudo-prop of the forwardRef component is actually used.

Supporting forwardRef properly requires some deeper changes to Flow's React support, which @jbrown215 is working on.

@goodmind goodmind added the Superseded PRs that solve the same issue as a "superseding" PR that already landed label Jul 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Superseded PRs that solve the same issue as a "superseding" PR that already landed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing React.forwardRef definition
6 participants