Skip to content

Add typings #538

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

Closed
wants to merge 16 commits into from
Closed

Add typings #538

wants to merge 16 commits into from

Conversation

bbenezech
Copy link

@bbenezech bbenezech commented Nov 7, 2016

See #433 for discussion.

I tried using DT's but I found some issues (connect with no argument was not adding dispatch prop, and some API calls where not possible). I fixed the issues for the most common use cases, described below.
The rest of the API should work like it does with DT version.
Please note: always define templates for your connect calls or you will have issues/subpar type checking (inference engine will replace non-inferable templates type with {}).

API Ref that we should targeted as much as possible: https://github.com/reactjs/react-redux/blob/master/docs/api.md#examples

These are the supported use cases that I tested and that 100% type check without any error or leniency:

1. Inject dispatch and don't listen to store

import { Dispatch } from 'redux'
import { checkout } from '../actions'
type AppState = {}

interface OwnProps {
  cartId: number
}

interface P extends OwnProps {
  dispatch: Dispatch<AppState> // try removing it, you'll get an error
}

class Cart extends Component<P, {}> {
  this.props.dispatch(checkout(this.props.cartId))
}

export default connect<AppState, OwnProps>()(Cart)
  • AppState is used as a template for the Dispatch type, as required by redux typings
  • OwnProps exposes the component with its props signature, without complicated InferableComponentDecorator that won't allow me to inject dispatcher.

2. Hydrate action creators without subscribing to the store

import { Dispatch } from 'redux'
import { checkout } from '../actions'

interface OwnProps {
  cartId: number
}

interface DispatchProps {
  checkout: (cartId: number) => void
}

interface P extends OwnProps, DispatchProps {}

class Cart extends Component<P, {}> {
  this.props.checkout(this.props.cartId) // checkout is hydrated
}

connect<{}, OwnProps, DispatchProps>(null, { checkout })(Cart)

3. Inject dispatch and props from global state

import { Dispatch } from 'redux'
import { checkout } from '../actions'

type AppState = {
  items: {
    [cartId: number]: Item[]
  }
}

interface OwnProps {
  cartId: number
}

interface StateProps { 
  items: Item[],
}

interface P extends OwnProps, StateProps {
  dispatch: Dispatch<AppState>
}

class Cart extends Component<P, {}> {
  this.props.dispatch(checkout(this.props.cartId))
  this.props.items.each(item => ...)
}

export default connect<AppState, OwnProps, StateProps>((state, props) => ({
  // state, props and result hash are all completely safe
  items: state.items[props.cartId]
}))(Cart)

4. Inject props from global state and hydrate action creators

type AppState = {
  items: {
    [cartId: number]: Item[]
  }
}

interface OwnProps {
  cartId: number
}

interface StateProps {
  items: Item[],
}

interface DispatchProps {
  checkout: (cartId: number) => void
}

interface P extends OwnProps, StateProps, DispatchProps {}

class Cart extends Component<P, {}> {
  this.props.checkout(this.props.cartId)
  this.props.items.each(item => ...)
}

export default connect<AppState, OwnProps, StateProps, DispatchProps>((state, props) => ({
  items: state.items[props.cartId]
}), { checkout })(Cart)
  • AppState describes your App's state
  • OwnProps describes your component's outside props (the ones that the parent component should specify)
  • StateProps describes the component's props that come from the global state
  • DispatchProps describes the component's props that come from hydrated action creators

Honestly, I only use case (3.)

This should get us started. Please feedback!

}

interface MergeProps<TStateProps, TDispatchProps, TOwnProps> {
(stateProps: TStateProps, dispatchProps: TDispatchProps, ownProps: TOwnProps): TStateProps & TDispatchProps;
Copy link

Choose a reason for hiding this comment

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

That's not correct. mergeProps can return anything, it should be a type parameter of MergeProps as well.

Copy link
Author

Choose a reason for hiding this comment

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

Done

* - TDispatchProps: Result of MapDispatchToProps
* - TOwnProps: Props passed to the wrapping component
*/
export function connect<State, TOwnProps>(): ComponentDecorator<{ dispatch: Dispatch<State> }, TOwnProps>;
Copy link

Choose a reason for hiding this comment

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

I guess it shouldn't replace props with {dispatch}, but merge them

Copy link
Author

@bbenezech bbenezech Nov 8, 2016

Choose a reason for hiding this comment

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

This is what ComponentDecorator is doing, no issue here.

Copy link

Choose a reason for hiding this comment

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

It doesn't. ComponentDecorator in this case states that it attepts component with props {dispatch: Dispatch<State>} and returns component with props TOwnProps.

In reality the effect of connect is adding dispatch to other properties.

Copy link
Author

Choose a reason for hiding this comment

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

Replied below

options?: Options
): ComponentDecorator<TStateProps & TDispatchProps, TOwnProps>;

interface MapStateToProps<State, TStateProps, TOwnProps> {
Copy link

Choose a reason for hiding this comment

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

Why not have type parameters in the same order as in the signature?

Copy link
Author

@bbenezech bbenezech Nov 8, 2016

Choose a reason for hiding this comment

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

Done.


export function connect<State, TOwnProps, TStateProps>(
mapStateToProps: MapStateToProps<State, TStateProps, TOwnProps>,
): ComponentDecorator<TStateProps & { dispatch: Dispatch<State> }, TOwnProps>;
Copy link

Choose a reason for hiding this comment

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

Wrapped component should get all of TStateProps, TOwnProps and dispatch

Copy link
Author

Choose a reason for hiding this comment

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

Same as previous one. ComponentDecorator exposes the exported component with TOwnProps outside, with TStateProps & { dispatch: Dispatch<State> } satisfied by React-Redux.
I think you are possibly misreading what ComponentDecorator does or how which props of the HOC should be exposed outside.
I've tested that part extensively in my (big) app and it really behaves as supposed to.

Copy link

@aikoven aikoven Nov 8, 2016

Choose a reason for hiding this comment

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

const connector = connect<S, {foo: string}, {bar: number}>(...);

Connector in this case gets the type ComponentDecorator<{bar: number} & {dispatch: Dispatch}, {foo: string}>. The component we pass in would have props {bar: number} & {dispatch: Dispatch}. The component that comes out would have props {foo: string}.

But how connect really works is this: connected component accepts {foo: string} — that's correct. But the wrapped component will get all of {foo: string}, {bar: number} and {dispatch: Dispatch}.

Copy link
Author

Choose a reason for hiding this comment

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

Ok. I felt that not exposing state props and dispatch prop to parent component was actually a feature.
Is there a valid use case where we should let users overwrite HOC's props?
Or is it more that you feel type checking should try to stick to reality as much as possible?

Copy link
Author

Choose a reason for hiding this comment

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

If we do this:

ComponentDecorator<TStateProps & { dispatch: Dispatch<State> }, TStateProps & { dispatch?: Dispatch<State> } & TOwnProps>

We can make dispatch optional but not TStateProps.
User would have to make all its StateProps optional so that it won't be required when component is used in parent.

But semantically, TStateProps are not optional.

This is a wontfix IMO.

): ComponentDecorator<TStateProps & { dispatch: Dispatch<State> }, TOwnProps>;

export function connect<State, TOwnProps, TStateProps, TDispatchProps>(
mapStateToProps: MapStateToProps<State, TStateProps, TOwnProps>|null,
Copy link

Choose a reason for hiding this comment

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

Let's make another signature for null as first argument. We'd get stronger typings for this case.

Copy link
Author

@bbenezech bbenezech Nov 8, 2016

Choose a reason for hiding this comment

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

another signature for null as first argument

Sorry I don't get it.
Should I add a type that expresses the union of MapStateToProps<State, TStateProps, TOwnProps> and null?
What advantage would it have?

Copy link

Choose a reason for hiding this comment

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

I propose adding this instead of union type:

export function connect<State, TOwnProps, TDispatchProps>(
  mapStateToProps: null,
  mapDispatchToProps: ...
)

This is stronger because in this case TStateProps isn't variable, the only type it could be is {}.

Copy link
Author

Choose a reason for hiding this comment

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

Oh. Ok, sorry. Makes sense.

Copy link
Author

Choose a reason for hiding this comment

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

Done

): ComponentDecorator<TStateProps & TDispatchProps, TOwnProps>;

export function connect<State, TOwnProps, TStateProps, TDispatchProps>(
mapStateToProps: MapStateToProps<State, TStateProps, TOwnProps>|null,
Copy link

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Done

/**
* Any since not every ActionCreator returns the same Action
*/
interface MapDispatchToPropsObject<TDispatchProps> {
Copy link

Choose a reason for hiding this comment

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

This should be used as a possible type for mapDispatch

Copy link
Author

Choose a reason for hiding this comment

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

This have been removed totally as well.

* Any since not every ActionCreator returns the same Action
*/
interface MapDispatchToPropsObject<TDispatchProps> {
[name: string]: ActionCreator<TDispatchProps>;
Copy link

Choose a reason for hiding this comment

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

That's not true, type argument of ActionCreator is the type of created Action. Here it could be anything

Copy link
Author

Choose a reason for hiding this comment

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

This have been removed totally as well.

*/
export interface ProviderProps {
store?: Store<any>;
children?: ReactNode;
Copy link

Choose a reason for hiding this comment

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

That's not needed as children prop is automatically added to components defined as classes.

Copy link
Author

Choose a reason for hiding this comment

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

Good. Let's fix this.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

* generic parameter in Store here by using any as the type
*/
export interface ProviderProps {
store?: Store<any>;
Copy link

Choose a reason for hiding this comment

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

Is it really optional?

Copy link
Author

Choose a reason for hiding this comment

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

I guess not. This should be fixed.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

import { Store, Dispatch, ActionCreator } from 'redux';

interface ComponentDecorator<TOriginalProps, TOwnProps> {
(component: ComponentClass<TOriginalProps>|StatelessComponent<TOriginalProps>): ComponentClass<TOwnProps>;
Copy link

Choose a reason for hiding this comment

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

I found out that it's better to have StatelessComponent as the first type. Otherwise when you pass SFC typescript will think that it is a constructor function for component class.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@aikoven
Copy link

aikoven commented Nov 8, 2016

Please note: always define templates for your connect calls or you will have issues/subpar type checking (inference engine will replace non-inferable templates type with {}).

That's not generally true, the pattern I use everywhere is this:

type State = {foo: string};

const Component = connect(
  (state: State) => {
    return {
      bar: state.foo,
    };
  }
)(props => {
  props.bar  // inferred as `string`
})

This saves a lot of typing clutter.

Even if you connect component classes, you can omit type parameters if only you annotate arguments for mapState / mapDispatch:

const connector = connect(
  (state: MyState, props: MyProps) => {...}
)

This way connector would be a decorator for components accepting MyProps plus stuff returned from mapState / mapDispatch (inferred automatically). And returned component would accept MyProps.

@bbenezech
Copy link
Author

bbenezech commented Nov 8, 2016

@aikoven

const connector = connect(
  (state: MyState, props: MyProps) => {...}
)

You are not type checking StateProps return. If you typo your hash, you won't know it. Thus you actually want:

const connector = connect(
  (state: MyState, props: MyProps): StateProps  => {...}
)

But then why not simply

const connector<MyState, MyProps, StateProps> = connect(
  (stats, props)  => {...}
)

The advantage is you get an error if you forget one of the generic argument, whereas you might forget to type check return types, argument types and lose some type checking, hence my warning, that was mostly directed to newcomers.

@bbenezech
Copy link
Author

bbenezech commented Nov 8, 2016

Job's done! 🎉
Thanks for your time @aikoven ! Your review was terrifically spot on! 🎯

@timdorr
Copy link
Member

timdorr commented Nov 8, 2016

Can this actually get built against the next branch? It would be a good idea for typings to show up in a major, rather than as a patch (they could be argued as minor, but I think we'd run into complaints there too).

@aikoven
Copy link

aikoven commented Nov 9, 2016

You are not type checking StateProps return. If you typo your hash, you won't know it.

That's just a matter of taste. I prefer not to specify an interface for an object that is consumed just a few lines later. My point is that you don't have to always specify type parameters because TS is quite good in inferring return types.

If we do this:

ComponentDecorator<TStateProps & { dispatch: Dispatch<State> }, TStateProps & { dispatch?: Dispatch<State> } & TOwnProps>

That's not what I meant. The right one is:

ComponentDecorator<TStateProps & {dispatch: Dispatch<State>} & TOwnProps, TOwnProps>;

Own props passed from outside, State props and dispatch merged into and passed to the wrapped component.

aikoven
aikoven previously requested changes Nov 9, 2016
* - TStateProps: Result of MapStateToProps
* - TDispatchProps: Result of MapDispatchToProps
*/
export function connect<State, TOwnProps>(): ComponentDecorator<{ dispatch: Dispatch<State> }, TOwnProps>;
Copy link

Choose a reason for hiding this comment

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

Correct return type:

ComponentDecorator<{dispatch: Dispatch<State>} & TOwnProps, TOwnProps>;

Copy link
Author

Choose a reason for hiding this comment

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

Done


export function connect<State, TOwnProps, TStateProps>(
mapStateToProps: FuncOrSelf<MapStateToProps<State, TOwnProps, TStateProps>>,
): ComponentDecorator<TStateProps & { dispatch: Dispatch<State> }, TOwnProps>;
Copy link

Choose a reason for hiding this comment

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

Correct return type:

ComponentDecorator<TStateProps & {dispatch: Dispatch<State>} & TOwnProps, TOwnProps>;

Copy link
Author

Choose a reason for hiding this comment

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

Done

export function connect<State, TOwnProps, TStateProps, TDispatchProps>(
mapStateToProps: FuncOrSelf<MapStateToProps<State, TOwnProps, TStateProps>>,
mapDispatchToProps: FuncOrSelf<MapDispatchToPropsFunction<State, TOwnProps, TDispatchProps> | MapDispatchToPropsObject & TDispatchProps>
): ComponentDecorator<TStateProps & TDispatchProps, TOwnProps>;
Copy link

Choose a reason for hiding this comment

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

Correct return type:

ComponentDecorator<TStateProps & TDispatchProps & TOwnProps, TOwnProps>;

Copy link
Author

Choose a reason for hiding this comment

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

Done


export function connect<State, TOwnProps, TStateProps, TDispatchProps>(
mapStateToProps: FuncOrSelf<MapStateToProps<State, TOwnProps, TStateProps>>,
mapDispatchToProps: FuncOrSelf<MapDispatchToPropsFunction<State, TOwnProps, TDispatchProps> | MapDispatchToPropsObject & TDispatchProps>
Copy link

Choose a reason for hiding this comment

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

In a case of passing object to mapDispatchToProps, what is TDispatchProps? What's the difference with MapDispatchToPropsObject?

I think it should be a different signature for the case when mapDispatchToProps is object, something like this:

function connect<State, TOwnProps, TStateProps, TDispatchProps extends MapDispatchToPropsObject>(
  mapStateToProps: FuncOrSelf<MapStateToProps<State, TOwnProps, TStateProps>>,
  mapDispatchToProps: TDispatchProps
): ComponentDecorator<TStateProps & TDispatchProps & TOwnProps, TOwnProps>;

Copy link
Author

Choose a reason for hiding this comment

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

TDispatchProps may be set by the user. It's the list of dispatchable action creators he intends to use in the component.
MapDispatchToPropsObject is a constraint we add to make sure values of the hash are action creators.

Adding a different signature is problematic because the combination will be very large. Same issue as with mapStateToProps: null.

export function connect<State, TOwnProps, TDispatchProps>(
mapStateToProps: null,
mapDispatchToProps: FuncOrSelf<MapDispatchToPropsFunction<State, TOwnProps, TDispatchProps> | MapDispatchToPropsObject & TDispatchProps>
): ComponentDecorator<TDispatchProps, TOwnProps>;
Copy link

Choose a reason for hiding this comment

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

Correct return type:

ComponentDecorator<TDispatchProps & TOwnProps, TOwnProps>;

Also see comment above on the case when mapDispatchToProps is object.

Copy link
Author

Choose a reason for hiding this comment

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

Done

mapDispatchToProps: FuncOrSelf<MapDispatchToPropsFunction<State, TOwnProps, TDispatchProps> | MapDispatchToPropsObject & TDispatchProps>,
mergeProps: MergeProps<TOwnProps, TStateProps, TDispatchProps, TMergeProps>,
options?: Options
): ComponentDecorator<TStateProps & TDispatchProps, TOwnProps>;
Copy link

Choose a reason for hiding this comment

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

Correct return type:

ComponentDecorator<TMergeProps, TOwnProps>;

Also see comment above on the case when mapDispatchToProps is object.

Copy link
Author

Choose a reason for hiding this comment

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

Done

}

interface MergeProps<TOwnProps, TStateProps, TDispatchProps, TMergeProps> {
(ownProps: TOwnProps, stateProps: TStateProps, dispatchProps: TDispatchProps): TMergeProps;
Copy link

Choose a reason for hiding this comment

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

The order of arguments is incorrect, the right one would be: stateProps, dispatchProps, ownProps

Copy link
Author

Choose a reason for hiding this comment

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

Oh my... I thought I was dealing with named arguments...
Fixed

@timdorr
Copy link
Member

timdorr commented Nov 9, 2016

@bbenezech Can you rebase against next and switch out the base for this PR (hit Edit at the top of the page)? Thanks!

@bbenezech bbenezech changed the base branch from master to next November 9, 2016 19:13
@timdorr timdorr dismissed aikoven’s stale review November 9, 2016 20:33

Looks like everything's been addressed.

@timdorr
Copy link
Member

timdorr commented Nov 9, 2016

@bbenezech Looks like there's a conflict because the git branch isn't based on next. Can you rebase and force push your branch? I don't know if I can do that on my end with the collaborator permission on the PR. Thanks for working with me on this! 👍

@bbenezech bbenezech mentioned this pull request Nov 9, 2016
@bbenezech
Copy link
Author

bbenezech commented Nov 9, 2016

@timdorr I can't change my own branch in edit. I opened a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants