Resolve flow errors with ReactTestRenderer #7736

aweary · 2016-09-14T22:34:52Z

Resolves #7735

aweary · 2016-09-14T22:36:03Z

src/renderers/testing/ReactTestRenderer.js


 type ReactTestRendererJSON = {
  type: string,
  props: { [propName: string]: string },
-  children: Array<string | ReactTestRendererJSON>,
+  children: null | Array<string | ReactTestRendererJSON>,
+  $$typeof?: any


Flow kept giving me a warning about $$typeof if I didn't mark it as optional. I think it might be because we're using Object.defineProperty?

This is weird :(

zpao · 2016-09-14T22:37:12Z

src/renderers/testing/ReactTestTextComponent.js

@@ -15,6 +15,7 @@
 import type { ReactText } from 'ReactTypes';

 class ReactTestTextComponent {
+  _currentElement: ReactText;


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)

Yupp, of course 👍

@zpao I added the second class usage in the codebase last week, so it's probably consistent so far :p

aweary · 2016-09-14T22:38:52Z

src/renderers/testing/ReactTestRenderer.js

+    children: Object,
+    transaction: ReactTestReconcileTransaction,
+    context: Object,
+  ) => void;


These methods are inherited from ReactMultiChild. Is there a way tell flow that this class inherits from ReactMultiChild using the current method of calling Object.assign on the prototype? ReactMultiChild isn't typed yet, though.

There isn't. In this case, since there's only one Object.assign, could you replace with class ... extends ReactMultiChild which flow should understand

aweary · 2016-09-14T22:40:17Z

cc @zpao @vjeux I made a first attempt at fixing what I broke :)

All flow errors should be resolved, but I'm not sure about my approach so feedback is appreciated.

vjeux · 2016-09-14T22:47:26Z

src/renderers/testing/ReactTestRenderer.js

+    children: Object,
+    transaction: ReactTestReconcileTransaction,
+    context: Object,
+  ) => void;


There isn't. In this case, since there's only one Object.assign, could you replace with class ... extends ReactMultiChild which flow should understand

vjeux · 2016-09-14T22:47:38Z

src/renderers/testing/ReactTestRenderer.js


 type ReactTestRendererJSON = {
  type: string,
  props: { [propName: string]: string },
-  children: Array<string | ReactTestRendererJSON>,
+  children: null | Array<string | ReactTestRendererJSON>,
+  $$typeof?: any


This is weird :(

vjeux · 2016-09-14T22:49:04Z

src/renderers/testing/ReactTestMount.js

-type TestRendererOptions = {
-  createNodeMock: (element: ReactElement) => Object,
+export type TestRendererOptions = {
+  createNodeMock: (element: ReactElement<any>) => Object,


Can you put and remove <any>

import { ReactElement } from 'ReactElementType';

We're trying to use the internal type (and not the one provided by flow) inside of the codebase

Please change Object to either any, mixed or the proper type.

The problem with using Object is that it makes it look like it is flow typed but in practice flow doesn't know any of its attributes so it cannot verify anything about it.

We're trying to either have the correct type or mixed (and if mixed doesn't work then any)

vjeux · 2016-09-14T22:52:11Z

src/renderers/testing/ReactTestTextComponent.js

@@ -15,6 +15,7 @@
 import type { ReactText } from 'ReactTypes';

 class ReactTestTextComponent {
+  _currentElement: ReactText;


@zpao I added the second class usage in the codebase last week, so it's probably consistent so far :p

vjeux · 2016-09-14T23:02:59Z

src/renderers/testing/ReactTestRenderer.js

-class ReactTestComponent {
+class ReactTestComponent extends ReactMultiChild {
+  _currentElement: ReactElement;
+  _renderedChildren: null | Object;


vjeux · 2016-09-15T01:53:44Z

Looks like there's a test failing. Probably related to turning the Object.assign inheritance to extend :x

aweary · 2016-09-15T03:03:15Z

@vjeux looks like it's because ReactMultiChild isn't a class or a function, so extend doesn't work. https://jsfiddle.net/enym4n2x/1/

I can either revert to using Object.assign and add the inherited types back, or I can just make ReactMultiChild a class, assuming that doesn't break lots of things :)

aweary · 2016-09-15T03:29:50Z

I was able to get ReactMultiChild working as a class without too much hassle, let me know if this approach is alright with you guys @vjeux @zpao

vjeux

Sgtm, can you npm run build on both master and this to see the impact on file size?

aweary · 2016-09-15T05:42:51Z

@vjeux it does increase the build size a bit

   raw     gz Compared to master @ 864bc7b939ca8a93583087d1ff596cd8049b4162
     =      = build/react-dom-fiber.js
     =      = build/react-dom-fiber.min.js
  +867   +154 build/react-dom-server.js
  +303    +40 build/react-dom-server.min.js
  +867   +152 build/react-dom.js
  +304    +46 build/react-dom.min.js
     =      = build/react-with-addons.js
     =      = build/react-with-addons.min.js
     =      = build/react.js
     =      = build/react.min.js

I'd be happy to roll back the ReactMultiChild change and approach it differently if you think that would be better right now.

gaearon · 2016-09-15T13:11:13Z

+40 in min is fine.

vjeux · 2016-09-15T16:13:40Z

Shipit :)

gaearon · 2016-09-15T22:13:25Z

src/renderers/art/ReactART.js

@@ -93,7 +93,7 @@ function injectAfter(parentNode, referenceNode, node) {

 // ContainerMixin for components that can hold ART nodes

-const ContainerMixin = assign({}, ReactMultiChild, {
+const ContainerMixin = assign({}, ReactMultiChild.prototype, {



Is this a breaking change for ART?

It shouldn't be AFAIK, it's still extending the same methods. They just exist on a class now instead of an object.

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

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.

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 RN

I'm probably missing something obvious, but why would it? That doesn't touch ReactART or ReactMultiChild.

This fixes flow errors introduced by #7649, so I was just wondering if we'd cut a release that contained some flow errors.

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?

The issue was that flow didn't understand that ReactTestComponent was inheriting mountChildren and updateChildren from ReactMultiChild with Object.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.

Let's do that for now please. Thanks for following up. :)

zpao · 2016-10-04T01:07:40Z

Where did we end up on this shipping & react-art? I'm cherry-picking this into 15.4 and will ship in RC unless I hear otherwise.

sophiebits · 2016-10-04T01:39:25Z

As long as you pick #7757 too we're good.

zpao · 2016-10-04T01:54:04Z

That brings us back to the state post- #7598, which was semver-major. Presumably we need to get back to ReactMultiChild.Mixin for ART, which makes me question if we should try to take this at all or if I should look at backporting to fix flow.

This is a manual cherry-pick of 2 PRs, updated to handle differences in the stable branch: - c78464f - Resolve flow errors with ReactTestRenderer (#7736) - 7dfa01f - Revert ReactMultiChild to plain object (#7757)

Resolve flow errors with ReactTestRenderer

5a9480a

ghost added GH Review: review-needed CLA Signed labels Sep 14, 2016

aweary commented Sep 14, 2016

View reviewed changes

zpao reviewed Sep 14, 2016

View reviewed changes

aweary commented Sep 14, 2016

View reviewed changes

Add whitespace between types and methods

5242f49

ghost added the CLA Signed label Sep 14, 2016

vjeux reviewed Sep 14, 2016

View reviewed changes

vjeux suggested changes Sep 14, 2016

View reviewed changes

Brandon Dail added 2 commits September 14, 2016 17:56

extend ReactMultiChild instead of using Object.assign

73ed879

Use ReactElement type from ReactElementType

c5c559b

ghost added the CLA Signed label Sep 14, 2016

vjeux reviewed Sep 14, 2016

View reviewed changes

vjeux approved these changes Sep 14, 2016

View reviewed changes

vjeux added GH Review: accepted and removed GH Review: review-needed labels Sep 14, 2016

facebook-github-bot added the CLA Signed label Sep 14, 2016

Make ReactMultiChild a class

6ecc9ec

ghost added the CLA Signed label Sep 15, 2016

vjeux reviewed Sep 15, 2016

View reviewed changes

ghost added the CLA Signed label Sep 15, 2016

koba04 mentioned this pull request Sep 15, 2016

Refactor ShallowRenderer #7739

Merged

aweary merged commit c78464f into facebook:master Sep 15, 2016

gaearon reviewed Sep 15, 2016

View reviewed changes

aweary mentioned this pull request Sep 16, 2016

Object.assign({}, objOfType) causes property not found in object literal error facebook/flow#1805

Closed

zpao added semver-exempt and removed semver-exempt labels Sep 19, 2016

zpao modified the milestone: 15-next Sep 19, 2016

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve flow errors with ReactTestRenderer #7736

Resolve flow errors with ReactTestRenderer #7736

aweary commented Sep 14, 2016

aweary Sep 14, 2016

vjeux Sep 14, 2016

zpao Sep 14, 2016

aweary Sep 14, 2016

vjeux Sep 14, 2016

aweary Sep 14, 2016 •

edited

Loading

vjeux Sep 14, 2016

aweary commented Sep 14, 2016

vjeux Sep 14, 2016

vjeux Sep 14, 2016

vjeux Sep 14, 2016 •

edited

Loading

vjeux Sep 14, 2016

vjeux Sep 14, 2016

vjeux Sep 14, 2016

vjeux commented Sep 15, 2016

aweary commented Sep 15, 2016 •

edited

Loading

aweary commented Sep 15, 2016

vjeux left a comment

aweary commented Sep 15, 2016

gaearon commented Sep 15, 2016

vjeux commented Sep 15, 2016

gaearon Sep 15, 2016

aweary Sep 15, 2016

gaearon Sep 16, 2016

gaearon Sep 16, 2016

aweary Sep 16, 2016 •

edited

Loading

sophiebits Sep 16, 2016

aweary Sep 16, 2016

sophiebits Sep 16, 2016

aweary Sep 16, 2016

sophiebits Sep 16, 2016

zpao commented Oct 4, 2016

sophiebits commented Oct 4, 2016

zpao commented Oct 4, 2016

Resolve flow errors with ReactTestRenderer #7736

Resolve flow errors with ReactTestRenderer #7736

Conversation

aweary commented Sep 14, 2016

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

aweary Sep 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary commented Sep 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjeux Sep 14, 2016 • edited Loading

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 Sep 15, 2016

aweary commented Sep 15, 2016 • edited Loading

aweary commented Sep 15, 2016

vjeux left a comment

Choose a reason for hiding this comment

aweary commented Sep 15, 2016

gaearon commented Sep 15, 2016

vjeux commented Sep 15, 2016

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

aweary Sep 16, 2016 • edited Loading

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

zpao commented Oct 4, 2016

sophiebits commented Oct 4, 2016

zpao commented Oct 4, 2016

aweary Sep 14, 2016 •

edited

Loading

vjeux Sep 14, 2016 •

edited

Loading

aweary commented Sep 15, 2016 •

edited

Loading

aweary Sep 16, 2016 •

edited

Loading