Type ReactRef #7600

vjeux · 2016-08-29T00:38:52Z

Nothing out of the ordinary on this one.

vjeux · 2016-08-29T00:39:31Z

src/isomorphic/classic/element/ReactElementType.js

@@ -23,7 +23,7 @@ export type ReactElement = {
  $$typeof: any,
  type: any,
  key: any,
-  ref: any,
+  ref: string | (elem: ?HTMLElement) => void,


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.

vjeux · 2016-08-29T00:40:04Z

src/renderers/shared/stack/reconciler/ReactRef.js

@@ -56,20 +66,21 @@ ReactRef.shouldUpdateRefs = function(prevElement, nextElement) {
  // is made. It probably belongs where the key checking and
  // instantiateReactComponent is done.

-  var prevEmpty = prevElement === null || prevElement === false;


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

gaearon · 2016-08-29T09:52:08Z

src/renderers/shared/stack/reconciler/ReactRef.js

@@ -56,20 +66,21 @@ ReactRef.shouldUpdateRefs = function(prevElement, nextElement) {
  // 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.


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.

vjeux · 2016-08-30T18:39:10Z

(Added needs-revision based on @zpao feedback)

vjeux · 2016-08-30T20:36:27Z

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!

gaearon · 2016-08-30T20:43:21Z

src/renderers/shared/stack/reconciler/ReactRef.js

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


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.

vjeux · 2016-08-30T20:54:19Z

Added null | false in detachRef

Nothing out of the ordinary on this one.

vjeux · 2016-09-02T20:40:45Z

src/renderers/shared/stack/reconciler/ReactRef.js

    // 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)


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

vjeux · 2016-09-02T20:41:53Z

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

gaearon · 2016-09-02T20:58:20Z

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.

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

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 facebook:master Sep 2, 2016

acdlite pushed a commit to acdlite/react that referenced this pull request Sep 9, 2016

Type ReactRef (facebook#7600)

ddd80c4

Nothing out of the ordinary on this one.

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)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type ReactRef #7600

Type ReactRef #7600

vjeux commented Aug 29, 2016

vjeux Aug 29, 2016

gaearon Aug 29, 2016

gaearon Aug 29, 2016

vjeux Aug 29, 2016 •

edited

vjeux Aug 29, 2016

andreypopp Aug 29, 2016

vjeux Aug 29, 2016 •

edited

gaearon Aug 29, 2016

zpao Aug 30, 2016

vjeux Aug 30, 2016

vjeux Aug 29, 2016

gaearon Aug 29, 2016

vjeux Aug 30, 2016

vjeux Aug 30, 2016

gaearon Aug 30, 2016

sophiebits Aug 30, 2016

vjeux commented Aug 30, 2016

vjeux commented Aug 30, 2016

gaearon Aug 30, 2016

vjeux Aug 30, 2016

vjeux commented Aug 30, 2016

vjeux Sep 2, 2016

vjeux commented Sep 2, 2016

gaearon commented Sep 2, 2016

Type ReactRef #7600

Type ReactRef #7600

Conversation

vjeux commented Aug 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjeux Aug 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjeux Aug 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjeux commented Aug 30, 2016

vjeux commented Aug 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjeux commented Aug 30, 2016

Choose a reason for hiding this comment

vjeux commented Sep 2, 2016

gaearon commented Sep 2, 2016

vjeux Aug 29, 2016 •

edited

vjeux Aug 29, 2016 •

edited