Details
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/.flowconfig | ||
|---|---|---|
| 78 ↗ | (On Diff #49465) | There seem to be some differences between our file and the one in the react-native repo: https://github.com/facebook/react-native/blob/0.80-stable/.flowconfig Historically we've kept these synced. Have there been any changes upstream that we should bring into our .flowconfig? multi_platform looks interesting, as does ban_spread_key_props. I haven't done an exhaustive analysis, just a couple things that popped up when I looked. |
| native/.flowconfig | ||
|---|---|---|
| 78 ↗ | (On Diff #49465) | PS please make sure to consider not just the native/.flowconfig, but the .flowconfigs we have in other workspaces |
- multiplatform setting:
Multiplatform breaks ClearableTextInput:
Error ┈ components/clearable-text-input.react.ios.js:181:16
Cannot conform to common interface module because ClearableTextInput [1] is incompatible with ClearableTextInput [2] in
property default. Read the docs on Flow's multi-platform support for more information:
https://flow.org/en/docs/react/multiplatform. [incompatible-type]
components/clearable-text-input.react.ios.js
[1] 14│ class ClearableTextInput extends React.PureComponent<
:
178│ },
179│ });
180│
181│ export default ClearableTextInput;
182│
components/clearable-text-input.react.js
[2] 12│ class ClearableTextInput extends React.PureComponent<ClearableTextInputProps> {So Flow thinks those two classes are different and hence the error.
In the docs https://flow.org/en/docs/react/multiplatform/ they propose:
- adding interface file .js.flow with component type or React.ComponentType
- common interface file in .js
I’ve tried both. The problem is ClearableTextInput is a class component with some methods (getValueAndReset). Other components keep ref to it and call the methods. And it’s difficult to create a common interface in the new flow syntax.
I tried also searching in react-native repo for some examples and there is DrawerLayoutAndroid component with separate implementations for android and ios and some methods but they just added $FlowFixMe https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/Components/DrawerAndroid/DrawerLayoutAndroid.android.js#L305
It also doesn’t provide much benefit for us - ClearableTextInput is the only component with separate implementations and we don’t have several flowconfigs anyway and it’s already typechecked. The only benefit would be that flow would additionally check if those two implementations implement the same interface?
- added casting_syntax to all flowconfigs, I think it’s ok to use both (: Type) and (as Type) syntaxes
- added ban_spread_key_props to all react workspaces
- added react.runtime=automatic to all react workspaces since @babel/plugin-transform-react-jsx was updated
Multiplatform breaks ClearableTextInput
Thanks for explaining the issue in detail. I think it's okay to skip this, it doesn't seem worth the effort.
Requesting changes for some missing configs and inconsistent ordering below. Will accept as soon as it's addressed!
| desktop/.flowconfig | ||
|---|---|---|
| 12 | Why don't we have these configs here? react.runtime=automatic ban_spread_key_props=true | |
| lib/.flowconfig | ||
| 15–16 | Would be good to match ordering / whitespace of all the configs, so it's easier to see differences at-a-glance It seems like you inserted the new configs in various places Can you make them all match the ordering / whitspace of upstream .flowconfig from react-native, as best you can? | |
| desktop/.flowconfig | ||
|---|---|---|
| 12 | Desktop workspace don't use react (it's just electron wrapper and there is no react dependency in package.json). Those settings are related to react, so it doesn't make much sense to put them here. | |
| lib/.flowconfig | ||
| 15–16 | In every config casting_syntax=both is now after enums=true and react.runtime=automatic, ban_spread_key_props=true are at the end, like in flowconfig from react-native | |