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

Add flow types generation, fix flow types #207

Merged
merged 21 commits into from
Feb 20, 2019
Merged

Add flow types generation, fix flow types #207

merged 21 commits into from
Feb 20, 2019

Conversation

kangax
Copy link
Contributor

@kangax kangax commented Dec 5, 2018

Even though flow has gen-flow-files command, it's experimental and doesn't really work — facebook/flow#5871

People are using various workarounds for generating lib types — https://blog.kentcdodds.com/distributing-flow-type-definitions-for-node-and-browser-modules-3952ad38b357

One of the solutions is to create .flow files that are simply a copy of plasma source files. This increases npm package size significantly, and that might have been fine except that it doesn't really work. We bundle plasma into a single file and Flow doesn't know which modules are exported and how to annotate them.

What I've decided to do for now is to automatically generate flow-typed defs in plasma and use that in SS. This is generally what a lot of libraries do (react-redux, lodash, etc.)

My solution has issues though. I'm extracting types from all the .jsx files under src/ using jscodeshift (same util that's used in codemods). The classes are exported and the types that those classes might use (e.g. Tab, Item) are declared locally.

One of the issues is that references like ExternalSelect.propTypes are not currently resolved into proper types. For now, I replace them with Object but there might be a way to do it properly. [update: now fixed]

Similarly, we need to do something about typeof some_var — those need to be resolved properly and substituted. [update: now fixed]

Finally, all the types and classes are exported in the same scope so there's a chance of conflicts. I'm not sure what the best solution for this is; we could scope-rename the types (e.g. type Item from tabs.jsx becomes type tabs_Item) and replace all occurrences in types.

  • normalize events
  • resolve typeof ... references

Update: we might be able to use flow type-at-pos to resolve types with all the values

import type { ComponentType, ElementConfig, Node, Component } from 'react';

declare module '@wework-dev/plasma' {
declare type Data = { key: string, value: string };
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a { [key: string]: string }?

}
value: Object,
},
},
|};
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a mock (and roundabout) definition of SyntheticEvent<T>

name: string,
onChange: () => mixed,
onChange: (event: SyntheticEvent<HTMLInputElement>) => mixed,
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest evt. I think event should be considered a saved word.

onBlur: (event: { target: {value: Object} }) => void,
onChange: (event: { target: {value: Object} }) => void,
onBlur?: (event: { target: { value: Object } }) => void,
onChange?: (event: { target: { value: Object } }) => void,
Copy link
Contributor

Choose a reason for hiding this comment

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

SyntheticEvents?

|};

class TimePicker extends Component<Props> {
static displayName = 'Plasma@TimePicker';

handleBlur = (event: { target: {value: Object} }): void => {
handleBlur = (event: { target: { value: Object } }): void => {
const newTime = event.target.value;
Copy link
Contributor

Choose a reason for hiding this comment

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

These should also be corrected to event.currentTarget.value for Flow support.

@kangax kangax changed the title [WIP] Add flow types generation, fix flow types Add flow types generation, fix flow types Dec 10, 2018
.replace('<propTypes>', '<Object>')}

declare export class CountedTextInput extends React$Component<{ ...$PropertyType<TextInput, 'props'>, counterStyle: string }> { }
declare export class CountedTextArea extends React$Component<{ ...$PropertyType<TextArea, 'props'>, counterStyle: string }> { }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ideally we would generate decorated components automatically

Copy link
Contributor Author

@kangax kangax left a comment

Choose a reason for hiding this comment

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

@GeReV maybe instead of parsing all type files under src/components, I should go over index.js, see which components are exported, find their paths, and extract types from them?

declare export class CountedTextInput extends React$Component<{ ...$PropertyType<TextInput, 'props'>, counterStyle: string }> { }
declare export class CountedTextArea extends React$Component<{ ...$PropertyType<TextArea, 'props'>, counterStyle: string }> { }

declare export var LocationPin: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we automatically export all files under icons as strings?

@GeReV
Copy link
Contributor

GeReV commented Dec 11, 2018

@kangax, sounds good with either approach.

scripts/buildFlowTypeDefs.js Outdated Show resolved Hide resolved
@kangax kangax merged commit e095b83 into master Feb 20, 2019
@kangax kangax deleted the flow_types branch February 20, 2019 19:17
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.

2 participants