Type ReactRef by vjeux · Pull Request #7600 · react/react · GitHub

Nothing out of the ordinary on this one.

Expanding the types of ReactElement as I go along. Flow coverage tool in nuclide is super useful to see what flows sees in the code I flowify :)

Is there any way flow coverage could be integrated into flow CLI? Many people don't use Nuclide and when they try flow, they aren't aware flow coverage exists.

(I understand there's a separate command but it's easy to miss when you are learning or trying it for the first time. I didn't know it existed until someone mentioned it on Twitter a month ago.)

There's already a command flow coverage <file> but it just shows the summary which isn't really useful:

Would be awesome if it printed in ascii art what nuclide shows:

cc @thejameskyle

flow coverage --color
On Mon, 29 Aug 2016 at 19:04, Christopher Chedeau notifications@github.com
wrote:

In src/isomorphic/classic/element/ReactElementType.js
#7600 (comment):

@@ -23,7 +23,7 @@ export type ReactElement = {
$$typeof: any,
type: any,
key: any,

ref: any,

ref: string | (elem: ?HTMLElement) => void,

cc @thejameskyle https://github.com/thejameskyle

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/facebook/react/pull/7600/files/6a1166f44d77d267879a9e39b6cde9241a569043#r76634323,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAB3gk_ZuNizzgRHP7SeOrxVSKUMDotlks5qkwMFgaJpZM4JvD5_
.

Omg, @andreypopp it works!

Why isn’t it the default 😄

(elem: ?HTMLElement) => void isn't right - you can get a component instance passed in (in the case of refs on composite components).

I think we probably want to define this correctly as much as possible and not leave partially correct types on fields (probably fine to leave any on to-be type fields).

(elem: ?HTMLElement) => void isn't right - you can get a component instance passed in (in the case of refs on composite components).

Good catch! I need to figure out what the type of a component instance is.

I think we probably want to define this correctly as much as possible and not leave partially correct types on fields (probably fine to leave any on to-be type fields).

Agreed, I want anything to be typed to be fully correct.

flow is not smart enough to know that this check is flowing through the variable for the next condition

Would be nice to learn what those false positives are since we're touching this code.

This blames to 999b0f9

cc @spicyj, let me know if you have more context on this/any concern.

I think the idea is that if both prevElement == null || prevElement === false and nextElement == null || nextElement === false then the function will return true. This is a false positive because if it was empty and stays empty, there is no need to update refs. Probably not a big deal.

Yes, that's right.

(Added needs-revision based on @zpao feedback)

Removed the ref type on ReactInstanceType as we don't yet have a type for the user class and it wasn't accurate enough.

Typed the function as ReactInstance | null | false in order to ensure that undefined cannot slip in.

I'm looking for another review, thanks for the comments!

Can be null or false here too.

Good catch, I can't wait for flow to warn when you're doing checks that do not make sense based on the type you have.

Added null | false in detachRef

Nothing out of the ordinary on this one.

All the tests are passing but i'd love a second pair of eyes to know if this is safe

It turned out to be a meatier change when adding string, false and null to the mix :)

Looks right to me. If it turns out to be wrong but we didn’t have tests for that case, our fault (and we’ll add tests).

Nothing out of the ordinary on this one. (cherry picked from commit 31dd694)

Nothing out of the ordinary on this one.

vjeux reviewed Aug 29, 2016
View reviewed changes

ghost added GH Review: review-needed CLA Signed labels Aug 29, 2016

vjeux reviewed Aug 29, 2016
View reviewed changes

vjeux assigned gaearon Aug 29, 2016

gaearon reviewed Aug 29, 2016
View reviewed changes

gaearon added GH Review: accepted and removed GH Review: review-needed labels Aug 29, 2016

zpao added the Component: Core Utilities label Aug 29, 2016

vjeux added GH Review: needs-revision and removed GH Review: accepted labels Aug 30, 2016

vjeux force-pushed the type_ReactRef branch from 6a1166f to 2f4a6d1 Compare August 30, 2016 20:34

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Aug 30, 2016

gaearon reviewed Aug 30, 2016
View reviewed changes

vjeux force-pushed the type_ReactRef branch from 2f4a6d1 to a26e134 Compare August 30, 2016 20:53

Type ReactRef

54e09c9

Nothing out of the ordinary on this one.

vjeux force-pushed the type_ReactRef branch from a26e134 to 54e09c9 Compare September 2, 2016 20:39

vjeux reviewed Sep 2, 2016
View reviewed changes

gaearon added GH Review: accepted and removed GH Review: review-needed labels Sep 2, 2016

vjeux added the semver-exempt label Sep 2, 2016

vjeux added this to the 15-next milestone Sep 2, 2016

vjeux merged commit 31dd694 into react:master Sep 2, 2016

zpao modified the milestones: 15-next, 15.4.0 Oct 4, 2016

zpao pushed a commit that referenced this pull request Oct 4, 2016

Type ReactRef (#7600 )

206bc2a

Nothing out of the ordinary on this one. (cherry picked from commit 31dd694)

mrizwanashiq pushed a commit to mrizwanashiq/react that referenced this pull request Jun 25, 2026

Type ReactRef (react#7600 )

71c3b00

Nothing out of the ordinary on this one.

-              ReactRef.detachRefs = function(instance, element) {
+              ReactRef.detachRefs = function(
+                instance: ReactInstance,
+                element: ReactElement,

                   // If owner changes but we have an unchanged function ref, don't update refs
-                  (typeof nextElement.ref === 'string' &&
-                   nextElement._owner !== prevElement._owner)
+                  (typeof nextRef === 'string' && nextOwner !== prevOwner)

                 type: any,
                 key: any,
-                ref: any,
+                ref: string | (elem: ?HTMLElement) => void,

                 // is made. It probably belongs where the key checking and
                 // instantiateReactComponent is done.
-                var prevEmpty = prevElement === null || prevElement === false;

-                var nextEmpty = nextElement === null || nextElement === false;
                 return (
                   // This has a few false positives w/r/t empty components.

Type ReactRef by vjeux · Pull Request #7600 · react/react · GitHub

Type ReactRef - #7600

Type ReactRef#7600vjeux merged 1 commit intoreact:masterreact/react:masterfrom vjeux:type_ReactRefvjeux/react:type_ReactRefCopy head branch name to clipboard

Conversation

vjeux commented Aug 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux Aug 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux Aug 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux commented Aug 30, 2016

Uh oh!

vjeux commented Aug 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux commented Aug 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux commented Sep 2, 2016

Uh oh!

gaearon commented Sep 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Type ReactRef#7600
vjeux merged 1 commit into
react:masterfrom
vjeux:type_ReactRef

vjeux Aug 29, 2016 •
edited
Loading

vjeux Aug 29, 2016 •
edited
Loading