facebook / flow

Adds static typing to JavaScript to improve developer productivity and code quality.
https://flow.org/
MIT License
22.08k stars 1.85k forks source link

Flow unnecessarily warns about missing props when using inject in MobX #6777

Open mulholo opened 5 years ago

mulholo commented 5 years ago

I have been using MobX's provider which allows you to pass a store to a component without passing it down through each layer of props. Usually, therefore, it is possible to have a component which has no props passed in by its parent, yet has props passed in by @inject.

However, Flow counts this (perfectly fine) behaviour as an error, as shown here:

screen shot 2018-08-22 at 09 38 38

Would it be possible to get the two libraries to play nicely together?

(Also, I suspect, but am not sure, that this might also affect React's context functionality.)


An example component using @inject, for reference:

// SortPicker.js

type SortPickerProps = {
  DataViewStore: DataViewStore, // store is only prop and passed in by inject
};

@inject('DataViewStore')@observer
export default class SortPicker extends React.Component<SortPickerProps> {
  // ...
}
alexandersorokin commented 5 years ago

Currently decorators are not supported by Flow and 'mobx-react' is not well-typed.

To generate typings for 'mobx-react' I use following inject function:

import * as React from 'react';
import {inject as untypedInject, Provider as UntypedProvider} from 'mobx-react';
import type {Store1, Store2} from './stores';

type Stores = $ReadOnly<{
  store1: Store1,
  store2: Store2,
}>;

type MakeMixed = <-V>(V) => mixed;

export function inject<
    TProps: {},
    TWrappedComponentType: React.ComponentType<TProps>,
    TInjectedProps: $Shape<
      & {...TProps} // Strip covariance modifiers ($Shape doesn't support it)
      & TProps // Improve error messages about injecting invalid props for $Shape
    >,
  >(propsSelector: (Stores) => TInjectedProps): (TWrappedComponentType) =>
  React.ComponentType<
    $Diff<
      $Exact<
        React.ElementConfig<TWrappedComponentType> // Support defaultProps
      >,
      $ObjMap<
        $Exact<TInjectedProps>,
        MakeMixed // Support injecting subtypes
        >
      >
    > {
  return wrappedComponentType => untypedInject(propsSelector)(wrappedComponentType);
}

export const Provider: React.ComponentType<Stores> = UntypedProvider;

To use it:

  1. Import 'Provider' from module above instead of importing it from 'mobx-react' and initialize it with stores.
    
    import {Provider} from './typedInject';
    import type {Store1, Store2} from './stores';

const store1: Store1 = ...; const store2: STore2 = ...; const stores = {store1, store2};

ReactDOM.render( <Provider {...stores}>

, document.getElementById('root'));


2. Import 'inject' function/decorator from module above instead of importing it from 'mobx-react'. Currently you can not use 'inject' function as decorator if you inject any mandatory props. You can use it as decorator only if you inject default props and optional props.

import * as React from 'react'; import {observer} from 'mobx-react'; import type {Store1, Store2} from './stores'; import {inject} from './typedInject';

type Props = { +store1: Store1, +default: string, +mandatory: string, +optional: string | void, };

@observer class Component extends React.Component { static defaultProps = {default: '42'}; render = () => this.props.store1 + this.props.default + this.props.mandatory; }

// You can inject 'store1' from Mobx Stores to 'store1' in Props. // You can inject any declared props, including optional and default ones. So you can inject 'default' prop. // You can inject any subtype instead of required type. So you can inject store or variable with literal type 'message' to 'optional' prop. // You can not inject any extra props. Flow will complain about it. So you can not inject anything to 'extra' prop. const InjectedComponent = inject(({store1}) => ({store1})) (Component);

// To use it as decorator for your Component you can use syntax @inject(({store2}) => ({optional: store2}))

export default InjectedComponent;


3. Just use it:
// No error // No error. You can pass default and optional props but you are not required to do so. // Error. Flow complain about missing 'mandatory' prop. // Error. Flow complain about extra 'extra' prop. // Error. Flow complain about already injected 'store1' prop. ```
melissafzhang commented 5 years ago

Thanks for this @alexandersorokin ! It's been extremely helpful. Can you take a look at my typedInject that takes in store names as arguments? It doesn't seem to be throwing any errors when it should be. Thanks!

export function inject<
  TProps: {},
  TWrappedComponentType: React.ComponentType<TProps>,
  TInjectedProps: $Shape<
    { ...TProps } & TProps, // Strip covariance modifiers ($Shape doesn't support it) // Improve error messages about injecting invalid props for $Shape
  >,
>(
  ...storeNames: $Keys<Stores>[] // takes in strings that are names of stores as arguments
): TWrappedComponentType => React.ComponentType<
  $Diff<
    $Exact<
      React.ElementConfig<TWrappedComponentType>, // Support defaultProps
    >,
    $ObjMap<
      $Exact<TInjectedProps>,
      MakeMixed, // Support injecting subtypes
    >,
  >,
> {
  return wrappedComponentType =>
    untypedInject(...storeNames)(wrappedComponentType);
}
alexandersorokin commented 5 years ago

@melissafzhang, it does not throwing any errors because storeNames argument does not have any connection to TInjectedProps.

To make it works you should do some sort of black magic. Let's do it. Firstly, you need create several files.

  1. Create createObjectFromArray.js with content below. It's helper function for Pick.
    
    // @flow

const createObjectFromArray = (keys: $ReadOnlyArray<*>) => keys.reduce((object, key) => { object[key] = undefined; return object; }, {});

export default createObjectFromArray;


You should do not merge/move content of this file with/to other files. It will broke everything.

2.  Create `Pick.js` with content below. This utility type allows select some field of object and filter other.

// @flow

import createObjectFromArray from './createObjectFromArray';

export type Pick<TSource: $Subtype<{}>, Keys: $ReadOnlyArray<$Keys>> = $ObjMapi< $Call<typeof createObjectFromArray, Keys>,

(Key) => $ElementType, >; ``` 3. Create `TypedInject.js` with content below. It's provide typings for `Provider` and `inject` from `mobx-react`. There is black magic happens. ``` // @flow import * as React from 'react'; import {inject, Provider} from 'mobx-react'; import type {Pick} from '../Pick'; type MakeMixed = <-TField>(TField) => mixed; type MapStoresToProps = < TProps: {}, // Inject is only allowed for React component with actual Props. No undefined/void Props allowed TWrappedComponentType: React.ComponentType, // Support defaultProps TInjects: $Shape< & {...TProps} // Strip covariance modifiers ($Shape doesn't support it) & TProps // Improve error messages about injecting invalid props for $Shape >, > (mapStoresToProps: $ReadOnly<$Exact> => TInjects) // Make Stores read only in mapper function => TWrappedComponentType => React.ComponentType< $Diff< $Exact< // Disallow pass extra (unknown) props to injected component React.ElementConfig // Support defaultProps >, $ObjMap< // Support injecting subtypes $Exact, // Required for $Diff MakeMixed // Support injecting subtypes > > > // No State generic type because state should be invisible outside React component ; type SelectStores = < TProps: {}, // Inject is only allowed for React component with actual Props. No undefined/void Props allowed TWrappedComponentType: React.ComponentType, // Support defaultProps TSelectedStoreNames: $ReadOnlyArray<$Keys>, // Allowed store names is intersection of prop names and store names. > (...selectedStores: TSelectedStoreNames) => TWrappedComponentType => React.ComponentType< $Diff< $Exact< // Disallow pass extra (unknown) props to injected component React.ElementConfig // Support defaultProps >, $Exact< // Required for $Diff Pick<$Exact, TSelectedStoreNames>, >, > // No State generic type because state should be invisible outside React component > ; export default class TypedInject { Provider: React.ComponentType<$ReadOnly> = Provider; inject: & MapStoresToProps & SelectStores = (...selector) => component => inject(...selector)(component); } ``` 4. Create `inject.js` with content below. It will describe you all stores and apply typings for `inject` and `Provider`. ``` // @flow import TypedInject from './TypedInject'; import type {IStore1} from '../Stores/Store1'; import type {IStore2} from '../Stores/Store2'; import type {IStore3} from '../Stores/Store3'; import type {IStore4} from '../Stores/Store4'; type Stores = { store1: IStore1, store2: IStore2, store3: IStore3, store4: IStore4, }; export const {inject, Provider} = new TypedInject; export default inject; ``` You should replace `IStores1`, `IStores2`, `IStores3` and `IStores4` with your actual stores. 5. In your React rendering root you should import 'Provider' from `inject.js` instead of importing it from 'mobx-react' and initialize it with stores. It's guarantee that you pass all required stores to `Provider` because 'Provider' from `inject.js` is typed with your stores. 'Provider' from 'mobx-react' is not typed. ``` // @flow import * as React from 'react'; import ReactDOM from 'react-dom'; import App from './App'; import {Provider} from './inject'; const store1 = ...; const store2 = ...; const store3 = ...; const store4 = ...; ReactDOM.render( , document.getElementById('root'), ); ``` 6. Then you can create your React component in any file. For example, I will use that component: ``` // @flow import * as React from 'react'; import {observer} from 'mobx-react'; import type {IStore1} from '../Stores/Store1'; import type {IStore2} from '../Stores/Store2'; // No Store3 import import type {IStore4} from '../Stores/Store4'; type Props = { +store1: IStore1, +store2: IStore2, +store3: IStore4, // Make note about name and type mismatch +default: number, +mandatory: number, +optional: number | void, } @observer class Component extends React.Component { // You can add generic type for 'State' if you want static defaultProps = {default: 42}; render = () => this.props.store1 + this.props.store2 + this.props.store3 + this.props.default + this.props.mandatory; } ``` 7. To inject stores you should import 'inject' function/decorator from `inject.js` instead of importing it from 'mobx-react'. You can inject mandatory, optional and default props. ``` // @flow import {inject} from './inject'; const m1 = inject(({store1}) => ({store1})); const m2 = inject(({store1, store2}) => ({store1, store2})); const m3 = inject(({store1}) => ({store1, store2: createStore2()})); // You are not required to use data to inject stores from provider only. const m4 = inject(({store1, store4}) => ({store1, store3: store4})); // Example of renaming store const m5 = inject(({store1}) => ({store1, store3: createStore4()})); const m6 = inject(({store1}) => ({store1, store2: createSubtypeOfStore2()})); // You may pass subtype instead of type const m7 = inject(({store1}) => ({store1, mandatory: 3, default: 3, optional: 3})); // You can inject constants const m8 = inject(({store1, store4}) => ({store1, mandatory: store4.someNumber, default: store4.someOtherNumber})); // You are not required to inject whole store. You can inject some observable parts of store. const n1 = inject('store1'); const n2 = inject('store2'); const n3 = inject('store1', 'store2'); // The following injects fail: const mFail1 = inject(({store1}) => ({store2: store1})); // Type of store2 (IStore2) doesn't match type of store1 (IStore1) const mFail2 = inject(({store1, store4}) => ({store1, store4})); // Props doesn't contains store4. It's only contains store1, store2 and store3 const mFail3 = inject(({store3}) => ({store3})); // Props type of store3 (IStore4) doesn't match provider type of store3 in provider (IStore3) const mFail4 = inject(({store1}) => ({store1, mandatory: 'str'})); // Type of mandatory prop (number) is not string const mFail5 = inject(({store1}) => ({store1, extraPropInjected: 55})); // Props doesn't contains 'extraPropInjected'. const mFail6 = inject(({store1, store666}) => ({store1, store3: store666})); // Provider doesn't contains store666. // The following injects also fail: const nFail1 = inject('store1', 'store4'); // Props doesn't not contains 'store4' const nFail2 = inject('store1', 'mandatory'); // There are no store with name 'mandatory' const nFail3 = inject('store1', 'extra'); // Props does not contains 'extra' and there are not store with name 'extra' const nFail4 = inject('store1', 'store3'); // type of store3 (IStore4) in Props doesn't match type of store3 in provider (IStore3) ``` 8. Then you can use injected components with following guidelines: a. You should pass all mandatory props that are not injected. b. You may pass props with default values given they are not injected. You may not pass props with default values if you are not want to do so. c. You may pass optional props given they are not injected. You may not pass optional props if you are not want to do so. d. You should pass type or subtype of prop. So you can pass 42 to type 'number' and pass 'hello' to type 'string | void | null'. e. You may not pass supertype of prop. So you can not pass variable with type 'number | void' to prop with type 'number'. f. You can not pass any injected prop. g. You can not pass any prop that are not declared in original Props of React component. Additional notes: 1. Currently you can not use 'inject' function as decorator if you inject any mandatory props. You can use it as decorator only if you inject default props and optional props. I still waiting for decorators proposal implemented by Flow. It can resolve this issue. 2. Sometimes there are one unsoundness happents if injecting stores using it's names. To make unsound inject you should create store and props with same name but different types. And type in Props should be subtype of type in Store. Flow will report no error when you inject store with that name. That is unsound. Moreover if type in Props is supertype of type in Store Flow will report error although there are no unsoundness. You can notice this if you are injecting optional/default props or optional stores. Flow will allow inject optional store to mandatory props (this is unsound). Flow will not allow inject store to optional or default prop unless store itself is optional (this should not be an error). The reason of that behavior is nature of $Diff. In that scenario it works in undesired way. There are no unsoundness if store and prop types are completely unrelated. There are no unsoundness if you inject stores with mapper function. So to ensure soundness your should use mapper function to inject optional and default props. 3. You should use Babel 7 to support syntax `export const {inject, Provider} = new TypedInject` in `inject.js`. If you are not use Babel 7 use can use syntax `export const {inject, Provider} = (new TypedInject: TypedInject)`.
melissafzhang commented 5 years ago

Thanks @alexandersorokin! How do you recommend typing the observer function from mobx-react?

We frequently have this pattern: export default inject('user')(observer(Component));

However, this causes the inject typing to break.

Thanks again!

melissafzhang commented 5 years ago

Here's my first pass at it

// @flow
import { observer as untypedObserver } from 'mobx-react';

type observer = <T: React.ComponentType<*>>(component: T) => T;

const typedObserver: observer = component => {
  return untypedObserver(component);
};

export default typedObserver;
alexandersorokin commented 5 years ago

@melissafzhang why do you need typing of the observer?

observer does not change type of wrapped component. So you can use it as decorator freely.

Also you can try something like this:

declare module "mobx-react" {
  import type {ComponentType} from "react";
  declare module.exports: {
    observer<T: ComponentType<*>>(component: T) => T;
  }
}

Or you can even modify proposed above TypedInject with:

export default class TypedInject<TStores: {}> {
  Provider: React.ComponentType<$ReadOnly<TStores>> = Provider;

  inject:
    & MapStoresToProps<TStores>
    & SelectStores<TStores>
      = (...selector) => component => inject(...selector)(observer(component));
}
melissafzhang commented 5 years ago

Thanks again @alexandersorokin! We want to allow passing in unknown props since we use the withRouter hoc from react-router which injects multiple props which might not be used. I removed the $Exact typings but still getting the error that the other injected props are undefined.

Cannot call withRouter because:
 • undefined property history [1] is incompatible with RouterHistory [2].
 • undefined property match [1] is incompatible with Match [3].
melissafzhang commented 5 years ago

Also I'm not sure if this is the intended behavior below. Here's a simplified example

type Props = {|
  myStore: MyStoreType
|}
export const InjectedComponent: React.ComponentType<Props> = inject(
  'myStore',
)(Component);

This throws the flow error;

Cannot assign `inject(...)(...)` to `InjectedComponent` because property `myStore` is missing in `Props` [1] but exists in `Props` [2] in type argument `P` [3]
kdrich commented 5 years ago

This is a good workaround reference: https://gist.github.com/vonovak/29c972c6aa9efbb7d63a6853d021fba9

Does the Flow team plan to add decorator support anytime soon?