Support flow type spread #241

elicwhite · 2018-01-05T00:39:20Z

Supporting type spread props:

type Props = {|
  /**
   * Spread props potentially from another component
   */
  ...OtherProps,

  /**
   * The first prop
   */
  prop1: string,
|}

This is a stacked PR on top of #240

elicwhite · 2018-01-05T20:36:31Z

It seems like we should change the getFlowType function to return something special for ObjectTypeSpread.

danez · 2018-01-05T23:26:31Z

I merged #240, could you please rebase?

elicwhite · 2018-01-05T23:30:40Z

Rebased. The right approach for this PR was a little less obvious to me. I tried to follow the algorithm conventions that existed but I'm not sure if the output of the spread type is what it actually should be. I would appreciate a bit closer review of those pieces.

danez · 2018-01-05T23:47:14Z

src/utils/__tests__/getFlowType-test.js

+      type: 'object',
+      signature: {
+        properties: [
+          { key: 'OtherProps', value: { name: 'OtherProps', required: true } },


I think it is not really clear what this is just from the json alone. It could be also interpreted as

type a = { OtherProps: OtherProps };

What i would try to do is resolve OtherProps and if it resolves to an other ObjectType within the same file then I would just inline this in the result. We do the same when resolving stuff here https://github.com/reactjs/react-docgen/blob/master/src/utils/resolveObjectKeysToArray.js#L59..L67

In the case when it cannot be resolved I'm not sure what the best approach is. We could either ignore the spread completely then, which we currently do for regular non-type ObjectSpread. Or somehow add a special field in the output aka "inherits" (or some better name) which just contains the names of the Type that was spreaded into the object. If I think about it spreading types is just some kind of multi inheritance and it is probably interesting to know.

{ name: 'signature', type: 'object', includes/inherits/implements/??: [ 'OtherProps' ], signature: { properties: [ { key: 'a', value: { name: 'string', required: true } }, ] } }

Not sure if that makes sense though.

The logic for resolving the local types is already being done. You can see it in action in the component snapshot test, prop OtherLocalProps.

Yeah, the real question is what to do when the type is imported from another file, 90% of our use case internally. We definitely don't want to ignore the spread. I think for us at least, saying what the types are is pretty explanatory. It's typically things like ...AccessibilityProps.

An explicit inherits field is interesting. I am not familiar enough with this project and how we consume the output to know what is ideal here. I think I defer to you.

This is an example of a component where we type spread props in and how we'd probably expect them to show up in docs. https://facebook.github.io/react-native/docs/touchableopacity.html#props

elicwhite · 2018-01-05T23:56:52Z

src/__tests__/__snapshots__/main-test.js.snap

+      },
+      "required": false,
+    },
+    "OtherLocalProps": Object {


It seems like this could be confused with a prop called OtherLocalProps that has these specific keys. However, since it is being spread in, the properties of OtherLocalProps should probably be flattened into the top level props? Is that correct?

elicwhite · 2018-01-08T19:42:06Z

@danez, do you have any more thoughts on these open questions?

elicwhite · 2018-01-09T19:35:33Z

I'm thinking that we want to distinguish between the spread properties and the normal ones. Also, since we want to resolve local types and possibly at some point have a way to reference where the definition originally comes from, we want the inherited list to be objects instead of plain strings.

I'm thinking this structure:

 it('handles ObjectTypeSpreadProperty', () => {
    var typePath = statement(`
      var x: { ...OtherProps, ...MyType, a: string };

      type MyType = { myString: string };
    `).get('declarations', 0).get('id').get('typeAnnotation').get('typeAnnotation');

    expect(getFlowType(typePath)).toEqual({
      name: 'signature',
      type: 'object',
      signature: {
        inheritedProperties: [
          { name: 'OtherProps', value: { name: 'OtherProps' } },
          { name: 'MyType',
            value: {
              name: 'signature',
              type: 'object',
              signature: {
                properties: [
                  { key: 'myString', value: { name: 'string', required: true } },
                ],
              },
              raw: '{ myString: string }',
            },
          },
        ],
        properties: [
          { key: 'a', value: { name: 'string', required: true } },
        ],
      },
      raw: '{ ...OtherProps, ...MyType, a: string }'});
  });

elicwhite · 2018-01-09T22:47:39Z

src/__tests__/__snapshots__/main-test.js.snap

+      "flowType": Object {
+        "name": "OtherComponentProps",
+      },
+      "inherited": true,


These have a flag for whether it is inherited. This was cleaner than adding a new top level key to the output documentation.

elicwhite · 2018-01-09T22:49:34Z

Alright, I think this is good to go now. I broke this into two separate PRs to make it easier to review.

elicwhite · 2018-01-11T19:16:33Z

@danez, kind bump. @fkling is on vacation so I can't bug him internally to get this merged and released. :)

Other people on my team like @yungsters, have merge permissions here but only you and felix can publish a new version. If you'd like to add the facebook npm user to have publish permissions: https://www.npmjs.com/~fb, then we can take care of it.

danez · 2018-01-11T21:28:00Z

Why did you decide to do it in 2 different ways? Once with inheritedProps and once with a inherited flag?

elicwhite · 2018-01-11T22:43:16Z

I could have gone either way, I actually initially had them both as a separate inherited object. The reason I didn't was partly because the code necessary to add inherited props to a new inheritedProps field in the documentation class is pretty hairy. Adding an inherited flag there was the simplest thing to do and required the most surface level changes that would be most likely to be merged. I figured if we wanted to split them out later it would be easier going this direction than the other direction.

There are a few different reasons I made getFlowType have a separate inheritedProperties. Fundamentally, the output from this function seems different than the component documentation output. One is describing the props, one is describing the type of a generic variable in JavaScript. The properties array also had key whereas the key of an inherited property doesn't actually make sense so inheritedProperties use name.

All in all, I don't feel strongly either direction. I'm happy to change approach if someone with more context has a preference. I mostly just want to get Facebook's UIDocs to stop blowing up on these new flow types. Perhaps we can merge and ship a PR that does that and then we can figure out the best output for these additional types.

elicwhite · 2018-01-19T19:12:40Z

src/__tests__/__snapshots__/main-test.js.snap

@@ -267,6 +267,35 @@ Object {
  "displayName": "MyComponent",
  "methods": Array [],
  "props": Object {
+    "OtherComponentProps": Object {


Move this to composes

"NonExistentFile.OtherCommmponentProps"

if default

then "URI"

elicwhite · 2018-01-19T19:12:50Z

src/__tests__/__snapshots__/main-test.js.snap

+      },
+      "inherited": true,
+    },
+    "OtherLocalProps": Object {


Inline this into props.

elicwhite · 2018-01-19T22:10:34Z

src/handlers/flowTypeHandler.js

  }
+
+  setPropDescription(documentation, path);


Add a comment to this.

elicwhite · 2018-01-19T22:13:49Z

src/__tests__/__snapshots__/main-test.js.snap

  "description": "",
  "displayName": "MyComponent",
  "methods": Array [],
  "props": Object {
+    "OtherLocalProps": Object {


This is wrong. Not spread in far enough.

elicwhite · 2018-01-19T22:15:35Z

src/__tests__/fixtures/component_11.js

+import { OtherComponentProps as OtherProps } from 'NonExistentFile';
+
+type OtherLocalProps = {|
+  fooProp?: string,


Add and pull in a docblock comment here.

danez · 2018-01-22T11:28:24Z

Looks good to me now. So you now changed it to use composes? That sounds really good to me. Should we try to align the other PR? composes and importedProperties... or do you think this are different usecases?

elicwhite · 2018-01-22T17:00:26Z

I think the other one needs a bit more work still. All of the inline comments I left were from doing an in person code review with @fkling. This PR should be good to go though. I’ll try to get Felix to review it closer today. :-)

danez · 2018-01-22T17:39:11Z

Okay I can also otherwise merge it . Sorry for the delay I'm really busy lately, but now off work for this week, so have more time.

fkling · 2018-01-22T18:25:51Z

src/handlers/flowTypeHandler.js

+        setPropDescriptor(documentation, propertyPath)
+      });
+    } else {
+      documentation.addComposes(name.node.name);


So this takes the variable name used in the spread syntax, right? Did we plan to use the module name instead at some point? I can't remember.

I think where it ended up was that it was too difficult to make something meaningful here.

import type { MyType } from 'MyFile'; type MyFileMyProperty = $PropertyType<MyType, 'myProperty'>; type Props = {| ...MyFileMyProperty |}

It doesn't make sense to say that it composes MyFile.MyFileMyProperty, in this case it would be MyFile.MyType.myProperty but the logic can get exceedingly complicated and it doesn't make sense for us to try to understand it statically. I think where we landed was to just use the alias, if they want it to be more explicit in the generated docs they can use a more explicit alias.

This still leaves us open to improving it at a future point though.

yungsters requested review from fkling and danez January 5, 2018 17:04

danez reviewed Jan 5, 2018

View reviewed changes

elicwhite commented Jan 5, 2018

View reviewed changes

elicwhite commented Jan 9, 2018

View reviewed changes

Support Flow Type Spread Props

54e709e

elicwhite force-pushed the support-flow-type-spread branch from d6b9300 to 54e709e Compare January 9, 2018 23:07

elicwhite mentioned this pull request Jan 9, 2018

Support deep objects with spread flow types #242

Closed

elicwhite mentioned this pull request Jan 11, 2018

Ignore ObjectTypeSpreadProperty #243

Closed

elicwhite commented Jan 19, 2018

View reviewed changes

Addressing feedback

1f26712

elicwhite commented Jan 19, 2018

View reviewed changes

src/handlers/flowTypeHandler.js Outdated

}

setPropDescription(documentation, path);

Copy link

Collaborator Author

elicwhite Jan 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment to this.

elicwhite commented Jan 19, 2018

View reviewed changes

elicwhite added 5 commits January 19, 2018 15:35

Properly spreading in locally resolved types

4ed3cd9

Clean up comment

3c5e905

Pass Flow

0768c75

Sort imports

f49091c

Update tests

71b2574

fkling reviewed Jan 22, 2018

View reviewed changes

fkling merged commit dace500 into reactjs:v3-dev Jan 23, 2018

elicwhite deleted the support-flow-type-spread branch January 23, 2018 00:44

github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022

Uh oh!

Support flow type spread #241

Support flow type spread #241

Uh oh!

Conversation

elicwhite commented Jan 5, 2018

Uh oh!

elicwhite commented Jan 5, 2018

Uh oh!

danez commented Jan 5, 2018

Uh oh!

elicwhite commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elicwhite commented Jan 8, 2018

Uh oh!

elicwhite commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elicwhite commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elicwhite commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danez commented Jan 11, 2018

Uh oh!

elicwhite commented Jan 11, 2018

Uh oh!

elicwhite Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danez commented Jan 22, 2018

Uh oh!

elicwhite commented Jan 22, 2018

Uh oh!

danez commented Jan 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elicwhite commented Jan 5, 2018 •

edited

Loading

elicwhite commented Jan 9, 2018 •

edited

Loading

elicwhite commented Jan 9, 2018 •

edited

Loading

elicwhite commented Jan 11, 2018 •

edited

Loading

elicwhite Jan 19, 2018 •

edited

Loading