Add flow types generation, fix flow types #207
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GeReV
reviewed
Dec 5, 2018
scripts/generateFlowTypes.js
Outdated
| import type { ComponentType, ElementConfig, Node, Component } from 'react'; | ||
|
|
||
| declare module '@wework-dev/plasma' { | ||
| declare type Data = { key: string, value: string }; |
There was a problem hiding this comment.
Shouldn't this be a { [key: string]: string }?
GeReV
reviewed
Dec 5, 2018
| } | ||
| value: Object, | ||
| }, | ||
| }, | ||
| |}; |
There was a problem hiding this comment.
Looks like a mock (and roundabout) definition of SyntheticEvent<T>
GeReV
reviewed
Dec 5, 2018
| name: string, | ||
| onChange: () => mixed, | ||
| onChange: (event: SyntheticEvent<HTMLInputElement>) => mixed, |
There was a problem hiding this comment.
I suggest evt. I think event should be considered a saved word.
GeReV
reviewed
Dec 5, 2018
| onBlur: (event: { target: {value: Object} }) => void, | ||
| onChange: (event: { target: {value: Object} }) => void, | ||
| onBlur?: (event: { target: { value: Object } }) => void, | ||
| onChange?: (event: { target: { value: Object } }) => void, |
GeReV
reviewed
Dec 5, 2018
| |}; | ||
|
|
||
| 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; |
There was a problem hiding this comment.
These should also be corrected to event.currentTarget.value for Flow support.
kangax
changed the title
kangax
commented
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 }> { } |
There was a problem hiding this comment.
ideally we would generate decorated components automatically
kangax
commented
Dec 10, 2018
| 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; |
There was a problem hiding this comment.
should we automatically export all files under icons as strings?
|
@kangax, sounds good with either approach. |
GeReV
reviewed
Feb 20, 2019
GeReV
approved these changes
Feb 20, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Even though flow has
gen-flow-filescommand, it's experimental and doesn't really work — facebook/flow#5871People 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
.flowfiles 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[update: now fixed]ExternalSelect.propTypesare not currently resolved into proper types. For now, I replace them withObjectbut there might be a way to do it properly.Similarly, we need to do something about[update: now fixed]typeof some_var— those need to be resolved properly and substituted.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 Itemfromtabs.jsxbecomestype tabs_Item) and replace all occurrences in types.typeof ...referencesUpdate: we might be able to use
flow type-at-posto resolve types with all the values