Definitelytyped: 5.0.16 Forces component to accept own props

[mikea]

This change: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/24764 breaks a lot of existing connect() calls that use ownProps.

The change now requires the connect component to accept TOwnProps (even though I rebind them to other properties in the component).

Can this change be rolled back?

@andnp @Pajn

[mikea]

Full code example:

https://codesandbox.io/s/044qkzjxmp

import * as React from "react";
import * as ReactRedux from "react-redux";

class Component extends React.Component<{
  firstName: string;
  lastName: string;
}> {
  public render() {
    return (
      <div>
        Hello, {this.props.firstName} {this.props.lastName}
      </div>
    );
  }
}

const ConnectedComponent = ReactRedux.connect(
  (
    state: {},
    ownProps: {
      data: {
        firstName: string;
        lastName: string;
      };
    }
  ) => {
    return {
      firstName: ownProps.data.firstName,
      lastName: ownProps.data.lastName
    };
  }
)(Component);

[andnp]

It looks like a bug with your code? Where is data coming from in:

ownProps: {
  data: {
    firstName: string;
    lastName: string;
  };
}

For what it's worth, this works:

import * as React from "react";
import * as ReactRedux from "react-redux";

class Component extends React.Component<{
  firstName: string;
  lastName: string;
}> {
  public render() {
    return (
      <div>
        Hello, {this.props.firstName} {this.props.lastName}
      </div>
    );
  }
}

const ConnectedComponent = ReactRedux.connect(
  (
    state: {},
    ownProps: {
      firstName: string;
      lastName: string;
    },
  ) => {
    return {
      firstName: ownProps.firstName,
      lastName: ownProps.lastName,
    };
  }
)(Component);

[mikea]

Data property should be supplied on connected component:

<ConnectedComponent data={firstName:”a”, lastName:”b”}/>

Your example works because your component already implements TOwnProps.
Another useful example of broken code is property rename. (first_name/firstName)

[mikea]

I've been reading react-redux documentation and it is not helpful:

https://github.com/reactjs/react-redux/blob/master/docs/api.md

However, the source code of react-redux does merge all props together:

https://github.com/reactjs/react-redux/blob/f892ec00d7e92ff7afb21498276472f0e3b000c5/src/connect/mergeProps.js#L4

Looks like I was wrong, and new typing better correspond to implementation.

[mikea]

I've been reading react-redux documentation and it is not helpful:

https://github.com/reactjs/react-redux/blob/master/docs/api.md

However, the source code of react-redux does merge all props together:

https://github.com/reactjs/react-redux/blob/f892ec00d7e92ff7afb21498276472f0e3b000c5/src/connect/mergeProps.js#L4

Looks like I was wrong, and new typing better correspond to implementation.

[tcharlat]

I strongly disagree to binding the decorated component prop interface to TOwnProps and would kindly require a revert to a separation of decorated/decoration implementation.

The decorated component should not know it is beeing decorated, it only requires to be instantiated with a set of required Props. It should know nothing about the state or how we need some ownProps to extract data from it.

In out use cases, when we decorate, mapStateToProps can require extra props (ex: to execute selectors).
These props are a requirement of mapStateToProps and should not leak to a dependency to the state API separated domain.

example:

I have a 'product' component, it only required the following props, with a simple implementation

export interface ProductProps {
  productName: string;
  price: number;
  currency: string;
  availability: number;
  addToBasket(num: number): void;
}

export const Product: React.StatelessComponent<ProductProps> = ({
  productName,
  price,
  currency,
  availability,
  addToBasket,
}) => (
  <div className="product-container">
    <ProductName name={productName} available={availability !== 0}/>
    <ProductPrice price={price} currency={currency}/>
    <ProductAddToBasket addToBasket={addToBasket}/>
  </div>
);

Now I want to connect this product to a state connect :

export interface ProductConnectOwnProps {
  productId: string;
}

export interface ProductConnectStateProps {
  productName: string;
  price: number;
  currency: string;
  availability: number;
}

function mapStateToProps(
  state: AppState,
  { productId }: ProductConnectOwnProps,
): ProductConnectStateProps {
  const currency = selectors.getUserCurrency(state);
  const { productName, prices, availability } = selectors.getProductById(
    state,
    productId,
  );
  const price = findPriceByCurrency(prices, currency);

  return { productName, price, availability, currency }
}

Now I can easily decorate my Product with a connect

const StateProduct = connect(mapStateToProps, mapDispatchToProps)(Product);

I can now use my StateProduct with only ownProps:

{
  productList.map(productId => (<StateProduct productId/>))
}

The latest update broke all our decorations, it forced us to leak things such as 'productId' in the 'ProductProps' interface.
But our product doesn't need an ID, it should not be aware of its existence.
We might use (and we do in some case) the Product without decoration, by passing it relevent props directly from another source, and faking a mocked productId string for the sake of satisfying a local decoration with redux-connect in another context is totally wrong !

[andnp]

I would suggest tagging a handful of the @types/react-redux authors here to make sure this issue is getting attention. I'm not actually knowledgeable on react-redux just typescript, so we are reaching the extent to which I am able to help debug this.