-
Notifications
You must be signed in to change notification settings - Fork 47.9k
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
Resolve flow errors with ReactTestRenderer #7736
Changes from all commits
5a9480a
5242f49
73ed879
c5c559b
6ecc9ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,11 +24,13 @@ var ReactTestTextComponent = require('ReactTestTextComponent'); | |
var ReactTestEmptyComponent = require('ReactTestEmptyComponent'); | ||
|
||
import type { ReactElement } from 'ReactElementType'; | ||
import type { ReactInstance } from 'ReactInstanceType'; | ||
|
||
type ReactTestRendererJSON = { | ||
type: string, | ||
props: { [propName: string]: string }, | ||
children: Array<string | ReactTestRendererJSON>, | ||
children: null | Array<string | ReactTestRendererJSON>, | ||
$$typeof?: any | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Flow kept giving me a warning about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is weird :( |
||
} | ||
|
||
/** | ||
|
@@ -46,8 +48,13 @@ function getRenderedHostOrTextFromComponent(component) { | |
return component; | ||
} | ||
|
||
class ReactTestComponent { | ||
class ReactTestComponent extends ReactMultiChild { | ||
_currentElement: ReactElement; | ||
_renderedChildren: null | Object; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
_topLevelWrapper: null | ReactInstance; | ||
|
||
constructor(element: ReactElement) { | ||
super(); | ||
this._currentElement = element; | ||
this._renderedChildren = null; | ||
this._topLevelWrapper = null; | ||
|
@@ -89,7 +96,7 @@ class ReactTestComponent { | |
childrenJSON.push(json); | ||
} | ||
} | ||
var object = { | ||
var object: ReactTestRendererJSON = { | ||
type: this._currentElement.type, | ||
props: props, | ||
children: childrenJSON.length ? childrenJSON : null, | ||
|
@@ -104,8 +111,6 @@ class ReactTestComponent { | |
unmountComponent(): void {} | ||
} | ||
|
||
Object.assign(ReactTestComponent.prototype, ReactMultiChild); | ||
|
||
// ============================================================================= | ||
|
||
ReactUpdates.injection.injectReconcileTransaction( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,8 @@ | |
import type { ReactText } from 'ReactTypes'; | ||
|
||
class ReactTestTextComponent { | ||
_currentElement: ReactText; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if we've been doing it consistently, but it would be good to give a bit of whitespace between types and actual JS - can you just put a newline under here (and in the other class too) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yupp, of course 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zpao I added the second |
||
|
||
constructor(element: ReactText) { | ||
this._currentElement = element; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a breaking change for ART?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be AFAIK, it's still extending the same methods. They just exist on a class now instead of an object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand but ART lives in its own repo. It's only here for running tests. (Yea I know it's confusing.) This means that if we had to change ART in a PR here, real ART will break with next version of React that contains this change. cc @spicyj @zpao
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words ART relies on private APIs (MultiChild), and private APIs just changed in this PR. Same for React Native until it starts using its own renderer copy (I think this hasn't happened yet).
Maybe I got it wrong but I think this is a breaking change for ART and RN and I'm not sure what we should do next.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining that, in that case it looks like this would be a breaking change. I can open PRs in RN and ART to update their use of MultiChild. Though I don't see and use of
ReactMultiChild
in RNThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably missing something obvious, but why would it? That doesn't touch ReactART or ReactMultiChild.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes flow errors introduced by #7649, so I was just wondering if we'd cut a release that contained some flow errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. I don't think that's terrible if we're confident that there are no bugs. But we should definitely find a way to fix flow errors before v16 since that isn't immediately coming. Maybe we can just remove flow from ReactTestRenderer or add some suppression comments for now if ReactMultiChild needs to be a class like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue was that flow didn't understand that ReactTestComponent was inheriting
mountChildren
andupdateChildren
from ReactMultiChild withObject.assign
.One way around that is what I did initially, which is just add annotations for those methods on ReactTestComponent directly. A suppression would work too. I'd be happy to revert the class conversion and fix the flow error somehow else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that for now please. Thanks for following up. :)