Conversation
| type: any, | ||
| key: any, | ||
| ref: any, | ||
| ref: string | (elem: ?HTMLElement) => void, |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(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 was a problem hiding this comment.
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:
There was a problem hiding this comment.
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_
.
There was a problem hiding this comment.
Why isn’t it the default 😄
There was a problem hiding this comment.
(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).
There was a problem hiding this comment.
(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.
| // is made. It probably belongs where the key checking and | ||
| // instantiateReactComponent is done. | ||
|
|
||
| var prevEmpty = prevElement === null || prevElement === false; |
There was a problem hiding this comment.
flow is not smart enough to know that this check is flowing through the variable for the next condition
| var nextEmpty = nextElement === null || nextElement === false; | ||
|
|
||
| return ( | ||
| // This has a few false positives w/r/t empty components. |
There was a problem hiding this comment.
Would be nice to learn what those false positives are since we're touching this code.
There was a problem hiding this comment.
cc @spicyj, let me know if you have more context on this/any concern.
There was a problem hiding this comment.
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.
|
(Added needs-revision based on @zpao feedback) |
|
Removed the Typed the function as I'm looking for another review, thanks for the comments! |
| ReactRef.detachRefs = function(instance, element) { | ||
| ReactRef.detachRefs = function( | ||
| instance: ReactInstance, | ||
| element: ReactElement, |
There was a problem hiding this comment.
Can be null or false here too.
There was a problem hiding this comment.
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 |
Nothing out of the ordinary on this one.
| // 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) |
There was a problem hiding this comment.
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.
Nothing out of the ordinary on this one. (cherry picked from commit 31dd694)
Nothing out of the ordinary on this one.