Flow-ify ReactPartialRenderer #11251

iamdustan · 2017-10-17T14:03:45Z

iamdustan · 2017-10-17T14:06:03Z

src/renderers/shared/server/ReactPartialRenderer.js

+  childIndex: number,
+  context: Object,
+  footer: string,
+  debugElementStack: Array<any>,


all the Frame annotations below only add debugElementStack in dev mode which causd a lot of errors in the logging utility functions. Just declaring it as always present (which it is in dev, and I think appropriately defended against in prod) solved a lot of the flow errors.

gaearon · 2017-10-17T20:05:24Z

src/renderers/shared/server/ReactPartialRenderer.js

@@ -320,6 +321,7 @@ function validateRenderResult(child, type) {

 function resolve(child, context) {
  while (React.isValidElement(child)) {
+    child = (child: React.Element);


Why is this needed?

I’m not really sure why Flow isn’t understanding that child is a React.Element at this point without this extra hint. I thought that by typing the callsites to resolve this would just work.

gaearon · 2017-10-17T20:05:36Z

src/renderers/shared/server/ReactPartialRenderer.js

      // Assume all trees start in the HTML namespace (not totally true, but
      // this is what we did historically)
      domNamespace: Namespaces.html,
      children,
      childIndex: 0,
      context: emptyObject,
      footer: '',
-    };
+    }: any);


all of these are because of #11251 (comment)

var frame = { ...everythingExcept_debugElementStack }; if (process.env.NODE_ENV) { frame.debugElementStack = []; }

So the frame: Frame = ({}: any) annotation is so that flow knows that this object can have the debugElementStack property.

var frame: Frame = ({ ...everythingExcept_debugElementStack // the any is to prevent Flow from considered this a // sealed object without the debugElementStack }: any);

without this then the following chunks of code have a lot of errors due to debugElementStack being undefined

https://github.com/iamdustan/react/blob/bacc1e72f58e1ec3eab16c52d6db28c7d0c21014/src/renderers/shared/server/ReactPartialRenderer.js#L65-L98

Typing something as any is generally as bad as not typing in the first place. Or possibly worse because now you have an illusion of type safety.

Could we perhaps use approach like

react/src/renderers/dom/fiber/ReactDOMFiberEntry.js

Lines 200 to 201 in 90370f2

if (__DEV__) {

const parentHostContextDev = ((parentHostContext: any): HostContextDev);

and

react/src/renderers/dom/fiber/ReactDOMFiberEntry.js

Line 210 in 90370f2

const parentNamespace = ((parentHostContext: any): HostContextProd);

?

Or just treat that field as always optional?

It is typed similarly, just var binding: Type = ({}: any); instead of var binding = (({}: any): Type);

Though I’ll look to into make the making separate *Prod and *Dev variants yet.

gaearon · 2017-10-17T20:05:46Z

src/renderers/shared/server/ReactPartialRenderer.js

            domNamespace: parentNamespace,
            children,
            childIndex: 0,
            context: context,
            footer: '',
-          };
+          }: Object);


gaearon · 2017-10-17T20:05:54Z

src/renderers/shared/server/ReactPartialRenderer.js

      domNamespace: getChildNamespace(parentNamespace, element.type),
      tag,
      children,
      childIndex: 0,
      context: context,
      footer: footer,
-    };
+    }: any);


gaearon

Let's try to get rid of any's.

gaearon · 2017-10-26T14:41:39Z

packages/react-dom/src/server/ReactPartialRenderer.js

@@ -319,6 +321,7 @@ function validateRenderResult(child, type) {

 function resolve(child, context) {
  while (React.isValidElement(child)) {
+    child = (child: React.Element);


This cast seemed suspicious to me. Because if it "worked" then Flow would've inferred it anyway.

I checked, and it seems like React.Element is any 😞 Flow can be so confusing. This is not the thing we're supposed to be using, apparently.

Yeah, one of my few wishes for flow is that it would be easier to see what it thought something was at any point in time and why.

gaearon · 2017-10-26T16:18:05Z

packages/react-dom/src/server/ReactPartialRenderer.js

-  constructor(element, makeStaticMarkup) {
+  stack: Array<Frame>;
+  exhausted: boolean;
+  currentSelectValue: null;


This is not a very helpful annotation. It's not always null. So by encoding it like this we actually make it harder to fix the Flow coverage because now it looks like fixing this is a Flow violation.

outdated

gaearon

I pushed a few changes.

gaearon · 2017-10-26T16:44:43Z

packages/react-dom/src/server/ReactPartialRenderer.js

  while (React.isValidElement(child)) {
+    // Safe because we just checked it's an element.
+    var element: ReactElement = ((child: any): ReactElement);


This is how to safely "cast" things to elements. (Ideally isValidElement() should work as a refinement but Flow folks haven't implemented that yet.)

I introduced a separate variable because you can't change the type of the existing one.

Do you need the : ReactElement on both sides of the assignment? I’ve only ever done one or the other and it always seemed to work as expected.

Not necessary. Can remove.

gaearon · 2017-10-26T16:45:09Z

packages/react-dom/src/server/ReactPartialRenderer.js

+  child: mixed,
+  context: Object,
+): {|
+  child: mixed,


Mixed is always better than any.

gaearon · 2017-10-26T16:45:39Z

packages/react-dom/src/server/ReactPartialRenderer.js

      childIndex: 0,
      context: emptyObject,
      footer: '',
    };
    if (__DEV__) {
-      topFrame.debugElementStack = [];
+      ((topFrame: any): FrameDev).debugElementStack = [];


This was the trick I meant to suggest. Keep both PROD and DEV paths typechecked.

I was trying (and unable) to find a way to get Flow to know that the only variant between Frame and FrameDev that could ever make it to the *CurrentDebugStack methods above were FrameDev types.

Not sure it's possible. I think using PROD type most things and then unsafely casting to DEV type for DEV things is the way to go.

iamdustan · 2017-10-27T13:30:03Z

packages/react-dom/src/server/ReactPartialRenderer.js

@@ -66,7 +66,8 @@ if (__DEV__) {
  var currentDebugStack = null;
  var currentDebugElementStack = null;
  var setCurrentDebugStack = function(stack) {
-    currentDebugElementStack = stack[stack.length - 1].debugElementStack;
+    var frame: Frame = stack[stack.length - 1];


since this entire chunk of code is inside a if (__DEV__) block I think we could safely say that var frame: FrameDev = stack[stack.length - 1];

I guess yea.

* flow ReactPartialRenderer * prettier * moving flow type around; * Move anys to DEV-only code path and keep it typechecked * Increase Flow coverage

facebook-github-bot added the CLA Signed label Oct 17, 2017

iamdustan commented Oct 17, 2017

View reviewed changes

iamdustan force-pushed the flow-react-partial-renderer branch 2 times, most recently from d8de171 to 6484d95 Compare October 17, 2017 14:28

gaearon reviewed Oct 17, 2017

View reviewed changes

gaearon previously requested changes Oct 18, 2017

View reviewed changes

iamdustan added 3 commits October 24, 2017 10:22

flow ReactPartialRenderer

2995ddb

prettier

3cd0096

moving flow type around;

c30783f

iamdustan force-pushed the flow-react-partial-renderer branch from bacc1e7 to c30783f Compare October 24, 2017 14:57

gaearon added 2 commits October 26, 2017 15:27

Merge branch 'master' into flow-react-partial-renderer

0adee58

Move anys to DEV-only code path and keep it typechecked

8011aa5

gaearon reviewed Oct 26, 2017

View reviewed changes

gaearon force-pushed the flow-react-partial-renderer branch 2 times, most recently from 720e328 to d9bab39 Compare October 26, 2017 16:48

gaearon changed the title ~~flow ReactPartialRenderer~~ Flow-ify ReactPartialRenderer Oct 26, 2017

gaearon reviewed Oct 26, 2017

View reviewed changes

Increase Flow coverage

12fcd71

gaearon force-pushed the flow-react-partial-renderer branch from d9bab39 to 12fcd71 Compare October 26, 2017 16:59

gaearon merged commit 80849fd into facebook:master Oct 26, 2017

iamdustan deleted the flow-react-partial-renderer branch October 27, 2017 13:28

iamdustan commented Oct 27, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flow-ify ReactPartialRenderer #11251

Flow-ify ReactPartialRenderer #11251

iamdustan commented Oct 17, 2017 •

edited

Loading

iamdustan Oct 17, 2017

gaearon Oct 17, 2017

iamdustan Oct 17, 2017

gaearon Oct 17, 2017

iamdustan Oct 17, 2017

gaearon Oct 17, 2017

iamdustan Oct 18, 2017

gaearon Oct 17, 2017

gaearon Oct 17, 2017

gaearon left a comment

gaearon Oct 26, 2017

iamdustan Oct 27, 2017

gaearon Oct 26, 2017

gaearon left a comment

gaearon Oct 26, 2017

iamdustan Oct 27, 2017

gaearon Oct 27, 2017

gaearon Oct 26, 2017

gaearon Oct 26, 2017

iamdustan Oct 27, 2017

gaearon Oct 27, 2017

iamdustan Oct 27, 2017

gaearon Oct 27, 2017

	if (__DEV__) {
	const parentHostContextDev = ((parentHostContext: any): HostContextDev);

Flow-ify ReactPartialRenderer #11251

Flow-ify ReactPartialRenderer #11251

Conversation

iamdustan commented Oct 17, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a 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

gaearon left a 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

Choose a reason for hiding this comment

iamdustan commented Oct 17, 2017 •

edited

Loading