microsoft / ReSub

A library for writing React components that automatically manage subscriptions to data sources simply by accessing them
MIT License
607 stars 49 forks source link

ComponentBase #100

Closed BiggA94 closed 5 years ago

BiggA94 commented 5 years ago

short: Is there any reason, that P in https://github.com/Microsoft/ReSub/blob/467399dcbbd14fd264b9ef5a0a10f9d3acbf60c1/src/ComponentBase.ts#L44 extends React.Props instead of just {}, as the Component in the Typedefinition of React does? See: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/cc6de05e3347a9ad3651df02f4b87603758e4398/types/react/index.d.ts#L397

Long form: I want to user Material-UI together with ReSub. Material-UI uses augmenting functions for the Properties. E.g.

import { WithStyles, createStyles } from '@material-ui/core';

const styles = (theme: Theme) => createStyles({
  root: { /* ... */ },
  paper: { /* ... */ },
  button: { /* ... */ },
});

interface Props extends WithStyles<typeof styles> {
  foo: number;
  bar: boolean;
}

So the Component would extend ComponentBase<Props, State>, but this gives a Compilation error, as in https://github.com/Microsoft/ReSub/issues/22. Extending React.props does only work, if on would leave the WithStyles augmentation. It adds the following:

interface Props  {
  foo: number;
  bar: boolean;
  classes: {
    foo: string,
    bar: string
  }
}

Here, one would loose this convenience method..

The Official Types definition for React.Component (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/cc6de05e3347a9ad3651df02f4b87603758e4398/types/react/index.d.ts#L397) defines the following:

 interface Component<P = {}, S = {}, SS = any> extends ComponentLifecycle<P, S, SS> { }

Is there a particular Reason for having

ComponentBase<P extends React.Props<any>, ...

instead of just

ComponentBase<P extends {}, ...

in https://github.com/Microsoft/ReSub/blob/467399dcbbd14fd264b9ef5a0a10f9d3acbf60c1/src/ComponentBase.ts#L44 ?

deregtd commented 5 years ago

Initial investigations are that this should be fine -- it appears that those props are now just dealt with in tooling. Checking with some affected teams within MS to confirm.

deregtd commented 5 years ago

We think it's going to be fine. Thanks for the contribution!

BiggA94 commented 5 years ago

Nice, Thank you.

PS: Are version upgrades only triggered manually?

deregtd commented 5 years ago

Published 1.1.0