Skip to content
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

187 support flow types #207

Merged
merged 49 commits into from
Feb 22, 2018
Merged

Conversation

mjclawar
Copy link
Contributor

Description

Supports creating docstrings using react-docgen for Dash components written solely with Flow type annotations. This is a non-breaking change.

The new logic, as determined in discussion in #187 is:

  1. If a Dash component has PropTypes-generated typing, the docstring uses the PropTypes, regardless of whether the component also has Flow types (current behavior).
  2. Otherwise if a Dash component has Flow types but not PropTypes, the docstring now uses the objects generated by react-docgen from the Flow types (new behavior).

Implementation

There are no breaking changes, and all pre-existing tests for the base_component suite still pass. Unit tests were added by @coralvanda for the docstrings generated by Flow. The unit tests only added another metadata.json file for a Flow-generated metadata.json test example, and a TestFlowMetaDataConversions TestCase.

As an example, the new Flow support on the Python dict in this gist will create this docstring:

"""
A MyComponent component.
test description

Keyword arguments:
- requiredUnion (string | number; required)
- optionalString (string; optional): A string that isn't required.
- optionalBoolean (boolean; optional): A boolean test
- optionalArray (list; optional): An array test with a particularly 
long description that covers several lines. It includes the newline character 
and should span 3 lines in total.
- requiredString (string; required): A required string
- optionalSignature(shape) (optional): This is a test of an object's shape. optionalSignature(shape) has the following type: dict containing keys 'checked', 'children', 'customData', 'disabled', 'label', 'primaryText', 'secondaryText', 'style', 'value'.
  Those keys have the following types: 
  - checked (boolean; optional)
  - children (a list of or a singular dash component, string or number; optional)
  - customData (bool | number | str | dict | list; required): A test description
  - disabled (boolean; optional)
  - label (string; optional)
  - primaryText (string; required): Another test description
  - secondaryText (string; optional)
  - style (dict; optional)
  - value (bool | number | str | dict | list; required)
- requiredNested (required): . requiredNested has the following type: dict containing keys 'customData', 'value'.
  Those keys have the following types: 
  - customData (required): . customData has the following type: dict containing keys 'checked', 'children', 'customData', 'disabled', 'label', 'primaryText', 'secondaryText', 'style', 'value'.
    Those keys have the following types: 
    - checked (boolean; optional)
    - children (a list of or a singular dash component, string or number; optional)
    - customData (bool | number | str | dict | list; required)
    - disabled (boolean; optional)
    - label (string; optional)
    - primaryText (string; required)
    - secondaryText (string; optional)
    - style (dict; optional)
    - value (bool | number | str | dict | list; required)
  - value (bool | number | str | dict | list; required)
- optionalNode (a list of or a singular dash component, string or number; optional): A node test
"""

Thanks to @coralvanda, this now also supports nested object definitions for Flow, e.g. this section:

- requiredNested (required): . requiredNested has the following type: dict containing keys 'customData', 'value'.
  Those keys have the following types: 
  - customData (required): . customData has the following type: dict containing keys 'checked', 'children', 'customData', 'disabled', 'label', 'primaryText', 'secondaryText', 'style', 'value'.
    Those keys have the following types: 
    - checked (boolean; optional)
    - children (a list of or a singular dash component, string or number; optional)
    - customData (bool | number | str | dict | list; required)
    - disabled (boolean; optional)
    - label (string; optional)
    - primaryText (string; required)
    - secondaryText (string; optional)
    - style (dict; optional)
    - value (bool | number | str | dict | list; required)
  - value (bool | number | str | dict | list; required)

which is pulled recursively from the requiredNested prop found here in the gist.

Related issue

Closes #187
Bumps version to 0.20.1, assuming this is appropriate.

@chriddyp
Copy link
Member

very nice! will take a look at this this week. do you have an example of a Dash react component that uses Flow? am curious about what the user would need to modify after generating the boilerplate from the dash-components-archetype package.

@mjclawar
Copy link
Contributor Author

I don't believe a user would need to modify anything if you use the current boilerplate. The babel react preset already supports stripping Flow types. You would $ npm install flow-bin as a developer and set it up in your build environment (we use IntelliJ, so it has linting for it).

Here's a FlatButton component that currently has both Flow and PropTypes. With this PR, we would just delete the PropTypes and move the descriptions from PropTypes to above the current Flow type prop and everything would work the same. The sd-material-ui package would then have a breaking change for the next release, since we would not support a dash version for metadata.json parsing below a potential release via this PR.

As a side note, pylint is very aggressive and not playing nice with my PEP8 linting. I think I finally got it where the tests pass.

@mjclawar
Copy link
Contributor Author

@chriddyp Need anything else on this? Also ran the codemods to help out with optionally updating the bundled React version for dash-html-components and dash-core-components this weekend.

"'{}'".format(t)
for t in list(type_object['value'].keys())),
'Those keys have the following types: \n{}'.format(
'\n'.join(create_prop_docstring(
Copy link
Member

Choose a reason for hiding this comment

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

🙇 🙇 🙇 🙇

@chriddyp
Copy link
Member

@mjclawar and @coralvanda - Sorry for the delay on this one. I've finally found some time to review and this looks good to me. Many, many thanks for writing the thorough tests and taking the time to improve the existing code.

Is there anything else that you would like to add? Otherwise, I'll merge this tonight and make a new release.

I think that I'd like to make a separate repo containing a sample flowtypes component that I can include across Dash's end-to-end integration tests. I can do this separately and it shouldn't block this PR.

@chriddyp
Copy link
Member

I think that I'd like to make a separate repo containing a sample flowtypes component that I can include across Dash's end-to-end integration tests

I created this here: https://github.com/plotly/dash-flow-example. I'll add an integration test that includes these prop types once this PR is merged. Screenshot of the example app:
image

@mjclawar or @coralvanda - How would I convert these nested PropTypes into flow types?


    /**
     * A list of options
     * Multiline description
     */
    options: PropTypes.arrayOf(PropTypes.shape({
        /**
         * The label of the option
         */
        'label': PropTypes.oneOfType([PropTypes.string, PropTypes.number]),

        /**
         * The value of the option
         */
        'value': PropTypes.string,
    })),

    /**
     * A config object
     */
    config: PropTypes.shape({
        /**
         * The title option
         */
        'title': PropTypes.oneOfType([PropTypes.string, PropTypes.number]),

        /**
         * The value of the option
         */
        'value': PropTypes.string,
    }),

I'd like to add a couple of nested properties to the flow example: https://github.com/plotly/dash-flow-example/blob/master/src/components/ExampleFlowComponent.react.js#L3

chriddyp added a commit that referenced this pull request Feb 22, 2018
@chriddyp chriddyp mentioned this pull request Feb 22, 2018
@mjclawar
Copy link
Contributor Author

mjclawar commented Feb 22, 2018

@chriddyp nice!

 /**
     * A list of options
     * Multiline description
     */
    options: PropTypes.arrayOf(PropTypes.shape({
        /**
         * The label of the option
         */
        'label': PropTypes.oneOfType([PropTypes.string, PropTypes.number]),

        /**
         * The value of the option
         */
        'value': PropTypes.string,
    })),

    /**
     * A config object
     */
    config: PropTypes.shape({
        /**
         * The title option
         */
        'title': PropTypes.oneOfType([PropTypes.string, PropTypes.number]),

        /**
         * The value of the option
         */
        'value': PropTypes.string,
    }),

is equivalent to

{
  /** A list of options*/
  options: Array<{
    /** The label of the option */
    label: string | number, 
    /** The value of the option*/
    value: string,
  }>,
  /** A config object */
  config: {
    /** The title option*/
    title: string | number,
    /** The value of the option */
    value: string,
}

You also can create a temporary type object to use elsewhere throughout the code, which is the best part! Example:

type T_OPTION = {
  /** The label of the option */
  label: string | number, 
  /** The value of the option*/
  value: string,
};
type T_CONFIG = {
  /** The title option */
  title: string | number,
  /** The value of the option */
  value: string
};

...
{
  /** A list of options */
  options: Array<T_OPTION>,
  /** A config object */
  config: T_CONFIG,
}

Once this is updated, more than willing to pass along a couple of examples from our libraries for more Flow examples.

@mjclawar
Copy link
Contributor Author

Is there anything else that you would like to add? Otherwise, I'll merge this tonight and make a new release.

Nothing besides wishing there were also typescript support for completeness, but I have never used that and have no interest in learning tonight! I think this PR completely addresses the issue. Plenty of examples can follow in other repos, and I would guess that the https://plot.ly/dash/plugins page could benefit from some updates after this.

@chriddyp
Copy link
Member

Great! Nice work @mjclawar and @coralvanda 🍻

@chriddyp chriddyp merged commit 8478942 into plotly:master Feb 22, 2018
chriddyp added a commit that referenced this pull request Feb 22, 2018
@chriddyp
Copy link
Member

I'm going to wait until #212 passes tests before making the formal release.

@mjclawar
Copy link
Contributor Author

🤞

chriddyp added a commit that referenced this pull request Feb 22, 2018
* add flowtype integration test

sanity check for #207

* white space

* add screenshot test to aborted example
@chriddyp
Copy link
Member

so far so good! available in v0.21.0. Doing the final screenshot test sanity check with the dash userguide https://github.com/plotly/dash-docs/pull/69

@mjclawar
Copy link
Contributor Author

mjclawar commented Feb 22, 2018

@chriddyp see updated sd-material-ui package with examples of using Flow types.

e.g., Checkbox

type Props = {
  /** Checkbox is checked if true */
  checked?: boolean,
  /** Checkbox is disabled if true */
  disabled?: boolean,
  /** A callback for firing events to dash */
  fireEvent?: () => void,
  /** Overrides the inline-styles of the icon element */
  iconStyle?: Object,
  /** The element's ID */
  id: string,
  /** Overrides the inline styles of the input element */
  inputStyle?: Object,
  /** The text label for the checkbox */
  label?: string,
  /** Where the label will be placed next to the checkbox */
  labelPosition?: 'left' | 'right',
  /** Overrides the inline styles of the Checkbox element label */
  labelStyle?: Object,
  /** Dash callback to update props on the server */
  setProps?: () => void,
  /** Override the inline styles of the root element */
  style?: Object,
};

and Snackbar

type Props = {
  /** Label for the action on the snackbar */
  action?: Node,
  /**
   * The number of milliseconds to wait before automatically dismissing. If no value is specified
   * the snackbar will dismiss normally. If a value is provided the snackbar can still be dismissed
   * normally. If a snackbar is dismissed before the timer expires, the timer will be cleared.
   */
  autoHideDuration?: number,
  /** Override the inline styles of the body element */
  bodyStyle?: object,
  /** CSS class name of the root element */
  className?: string,
  /** Override the inline styles of the content element */
  contentStyle?: object,
  /** Dash event handler for click events */
  fireEvent?: () => void,
  /** The element's ID */
  id: string,
  /**
   * The message to be displayed.
   * (Note: If the message is an element or array, and the Snackbar may re-render while it is
   * still open, ensure that the same object remains as the message property if you want to avoid
   * the Snackbar hiding and showing again)
   */
  message: Node,
  /** An integer that represents the number of times that action button has been clicked */
  n_clicks?: number,
  /** Controls whether the Snackbar is opened or not */
  open: boolean,
  /** Dash callback to update props on the server */
  setProps?: (props: {open?: boolean}) => void,
  /** Override the inline styles of the root element */
  style?: object,
};

sbvhev added a commit to sbvhev/dash that referenced this pull request Mar 12, 2018
* add flowtype integration test

sanity check for plotly/dash#207

* white space

* add screenshot test to aborted example
denis554 added a commit to denis554/dash that referenced this pull request Apr 9, 2019
* add flowtype integration test

sanity check for plotly/dash#207

* white space

* add screenshot test to aborted example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants