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

[@types/react-redux] [enhancement] Connected component type inference #31227

Closed
Voronar opened this issue Dec 10, 2018 · 11 comments
Closed

[@types/react-redux] [enhancement] Connected component type inference #31227

Voronar opened this issue Dec 10, 2018 · 11 comments

Comments

@Voronar
Copy link

@Voronar Voronar commented Dec 10, 2018

When we work with TypeScript we have to manually declare prop types injected to connected component.
In this article I described how to do this work automatically with type inference.
Do you think about what? What about providing this utility type to main typings?

@Kallikrein

This comment has been minimized.

Copy link
Contributor

@Kallikrein Kallikrein commented Dec 10, 2018

Hello @Voronar !
I read your automatic type inference use case and as far as I understand, I think you assume that your decorated (inner) component props are all injected by mapStateToProps return.

With this assumption it's safe to infer inner component types, but there are cases where this assumption is false :

const MyInnerComponent: React.StatelessComponent<{foo: string; bar: string;}> = props => (
  <div>
    <div className="foo">{props.foo}</div>
    <div className="bar">{props.bar}</div>
  </div>
);

const myStateToProps = (state: AppState, ownProps: {baz: string}): {foo: string;} => ({
  foo: mySelector(state, ownProps.baz)
});

const MyOuterComponent = connect(myStateToProps)(MyInnerComponent);

The correct typing for MyOuterComponent is

let MyOuterComponent: React.ComponentType<{bar: string; baz: string;}>;

Here, connect correctly infers MyOuterComponent typings, but only because MyInnerComponent props are explicit.

In other words :

  • MyInnerComponent requires two props (foo and bar), one (foo) is injected through state and mapStateToProps
  • myOuterComponent require two props (bar and baz), one (baz) is a dependency of mapStateToProps and is required by mySelector
  • the bar props is 'untouched' by the decoration, it is a dependency of Inner, and since it's not injected by mapStateToProps it's still a dependency of Outer

If you wanted "not to have to" explicitly declare prop types of MyInnerComponent, the only way to have the 'bar' prop in both Inner and Outer would be to put it in mapStateToProps 'OwnProps', although mapStateToProps does not require this variable to correctly return state props.
BUT leaking this dependency in mapStateToProps (which will ignore the prop) is an error from a concern compartmentalization viewpoint, it forbids you from using your mapStateToProps on another component without the 'bar' dependency, although in another context it may be superfluous and ignored by the Inner2 component.

I do agree that when dealing with inferred types vs explicit types , arbitrating where to put the cursor to avoid unnecessary typings is a delicate task. IMHO It's best to have too much types (and in extenso more type checkings), that too few types and automagically inferred potential incoherence...

@Voronar

This comment has been minimized.

Copy link
Author

@Voronar Voronar commented Dec 11, 2018

Hello @Kallikrein !
I don't see the problem. Code you have write above is correctly work with TypeOfConnect utility:

const myStateToProps = (_: any, ownProps: {baz: string}): {foo: string} => ({
  foo: ownProps.baz,
});

const enhancer = connect(myStateToProps);

type Props = {
  bar: string;
}
  & TypeOfConnect<typeof enhancer>
;

const MyInnerComponent: React.StatelessComponent<Props> = props => (
  <div>
    <div className="foo">{props.foo}</div>
    <div className="bar">{props.bar}</div>
  </div>
);

const MyOuterComponent = enhancer(MyInnerComponent);

<MyOuterComponent bar="" baz="">1</MyOuterComponent>;
@surgeboris

This comment has been minimized.

Copy link
Contributor

@surgeboris surgeboris commented Dec 12, 2018

@Voronar Thank you very much for the article, I like the approach and I think that this utility type might be really usefull.

However I'm not sure if it is a good idea to include it into library typings as it's more like a "nice-to-have" thing than a really required part of the typings.

I mean anyone who wants to use it can just copy the definition from your article (it's pretty small), and even if we will ship this definition with @types/react-redux I'm pretty much sure that 99% of users will never notice that and will not use it (unless your article will be promoted so much that all of these 99% of users will read it, of course).

To sum it up: I think it will be more beneficial to promote the article itself then including TypeOfConnect higher-order-type into library typings; however if there will be a lot of developers who really want to have it included - I don't really mind.

@charrondev

This comment has been minimized.

Copy link
Contributor

@charrondev charrondev commented Dec 12, 2018

I think it would be beneficial to have it included. Utility types for dealing with the react/redux boundaries are always helpful!

@markerikson

This comment has been minimized.

Copy link

@markerikson markerikson commented Aug 6, 2019

Hi! I'm a Redux maintainer. And, uh, I'm finally starting to use Redux with TypeScript for realzies now.

I just ran into this issue myself, and I would strongly recommend that this "extract the connected props" type be included in the library. If it is, we can then document it as a recommended way to handle things.

@charrondev

This comment has been minimized.

Copy link
Contributor

@charrondev charrondev commented Aug 7, 2019

I will note that my current workaround tends to look something like this:

function MyComponent(props: IProps) {
   return <div /> 
}

interface IOwnProps {
   someParam: boolean;
}

function mapStateToProps(state: IStoreState) {
    return {
        // Return some stuff.
    }
}

function mapDispatchToProps(dispatch: IDispatch) {
    return {
        // Return some stuff.
    }
}

type IProps = IOwnProps & ReturnType<typeof mapStateToProps> & ReturnType<typeof mapDisaptchToProps>;

export default connect(mapStateToProps, mapDispatchToProps)(MyComponent);
@markerikson

This comment has been minimized.

Copy link

@markerikson markerikson commented Aug 7, 2019

Yeah, that's basically what I was doing, except that I was using the object shorthand form of mapDispatch:

const mapState = (state: RootState) => ({
    todos: state.todos,
})

const mapDispatch = {
    fetchTodosThunk,
}

interface OwnProps {
    someValue: string;
}

type MyComponentProps = ReturnType<mapState> & typeof mapDispatch & OwnProps;

But, this breaks because connect "unwraps" the thunk return types, so you get a mismatch: fetchTodosThunk is defined as () => SomeThunkType, while the unwrapped result is () => void.

By reusing the React-Redux internal types, we can change it to:

const mapState = (state: RootState) => ({
    todos: state.todos,
})

const mapDispatch = {
    fetchTodosThunk,
}

interface OwnProps {
    someValue: string;
}

const connectMyComponent = connect(mapState, mapDispatch);

type PropsFromRedux = ConnectedProps<typeof connectMyComponent>;
type MyComponentProps = PropsFromRedux & OwnProps;

class MyComponent extends Component<MyComponentProps> {}

export default connectMyComponent(MyComponent);

Still several steps, but at least it fixes the issues with using mapDispatch as an object, and simplifies the "props from Redux" part.

@markerikson

This comment has been minimized.

Copy link

@markerikson markerikson commented Oct 27, 2019

Hi, it's me again!

I would really like to have this added in to the React-Redux types, per https://gist.github.com/christianchown/05e084c78ec216070a5a2b80f0534d4b:

import { InferableComponentEnhancerWithProps } from 'react-redux';

export default type ConnectedProps<T> = T extends InferableComponentEnhancerWithProps<infer Props, infer _> ? Props : never;

Who owns the typedefs, and how do we make this happen?

@markerikson

This comment has been minimized.

Copy link

@markerikson markerikson commented Oct 27, 2019

Waitasec. The ConnectedProps type was already added in #37300 !

This issue can be closed, as the type now exists and was published in https://unpkg.com/browse/@types/react-redux@7.1.2/ .

@Voronar

This comment has been minimized.

Copy link
Author

@Voronar Voronar commented Oct 28, 2019

Thank you for adding the subject stuff here (
#37300).

@Voronar Voronar closed this Oct 28, 2019
@markerikson

This comment has been minimized.

Copy link

@markerikson markerikson commented Nov 23, 2019

The ConnectedProps<T> pattern is now described in the React-Redux docs, and we're officially recommending that people use it:

https://react-redux.js.org/using-react-redux/static-typing#typing-the-connect-higher-order-component

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.