Skip to content

Commit

Permalink
feat(react): improve props and state typesafety on Component
Browse files Browse the repository at this point in the history
- fix(react): remove failing expectation on TS 2.6
- chore(react): add myself to contributors
  • Loading branch information
Hotell committed Jun 26, 2018
1 parent 61a609c commit 542f3c0
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 3 deletions.
20 changes: 17 additions & 3 deletions types/react/index.d.ts
Expand Up @@ -17,6 +17,7 @@
// Ferdy Budhidharma <https://github.com/ferdaber>
// Johann Rakotoharisoa <https://github.com/jrakotoharisoa>
// Olivier Pascal <https://github.com/pascaloliv>
// Martin Hochel <https://github.com/hotell>
// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
// TypeScript Version: 2.6

Expand Down Expand Up @@ -281,13 +282,18 @@ declare namespace React {
// tslint:disable-next-line:no-empty-interface
interface Component<P = {}, S = {}, SS = any> extends ComponentLifecycle<P, S, SS> { }
class Component<P, S> {
constructor(props: Readonly<P>);
/**
* @deprecated
* https://reactjs.org/docs/legacy-context.html
*/
constructor(props: P, context?: any);

// We MUST keep setState() as a unified signature because it allows proper checking of the method return type.
// See: https://github.com/DefinitelyTyped/DefinitelyTyped/issues/18365#issuecomment-351013257
// Also, the ` | S` allows intellisense to not be dumbisense
setState<K extends keyof S>(
state: ((prevState: Readonly<S>, props: P) => (Pick<S, K> | S | null)) | (Pick<S, K> | S | null),
state: ((prevState: Readonly<S>, props: Readonly<P>) => (Pick<S, K> | S | null)) | (Pick<S, K> | S | null),
callback?: () => void
): void;

Expand All @@ -299,9 +305,17 @@ declare namespace React {
// always pass children as variadic arguments to `createElement`.
// In the future, if we can define its call signature conditionally
// on the existence of `children` in `P`, then we should remove this.
props: Readonly<{ children?: ReactNode }> & Readonly<P>;
state: Readonly<S>;
readonly props: Readonly<{ children?: ReactNode }> & Readonly<P>;
readonly state: null | Readonly<S>;

This comment has been minimized.

Copy link
@Bnaya

Bnaya Jun 28, 2018

Collaborator

Why State can always be null ?! this is a bad regression

This comment has been minimized.

Copy link
@kiyopikko

kiyopikko Jun 29, 2018

Contributor

I met 324 errors on my project because of this... 😫

This comment has been minimized.

Copy link
@Rycochet

Rycochet Jun 29, 2018

Contributor

While this is technically true that State can be null, this is in fact a breaking change in the typedefs. The state can only be null if you forget to set it in the constructor, and that is a very easy thing to find (especially as it's mentioned so often in the documentation).

Spending less than a month coding with Typescript + React doesn't qualify you to break both SemVer and pretty much everybody's projects...

/**
* @deprecated
* https://reactjs.org/docs/legacy-context.html
*/
context: any;
/**
* @deprecated
* https://reactjs.org/docs/refs-and-the-dom.html#legacy-api-string-refs
*/
refs: {
[key: string]: ReactInstance
};
Expand Down
67 changes: 67 additions & 0 deletions types/react/test/index.ts
Expand Up @@ -54,6 +54,72 @@ declare const container: Element;
//
// Top-Level API
// --------------------------------------------------------------------------
{
interface State {
inputValue: string;
seconds: number;
}
/**
* This is a "pre EcmaScript class property era" style of setting state within constructor.
* This occurs also if you need to provide som logic before mounting your component.
* To mitigate this error you need to provide state property definition upfront.
*/
class SettingStateFromCtorComponent extends React.Component<Props, State, Snapshot> {
// uncomenting this fixes the error :)
// state: State;
constructor(props: Props) {
super(props);
// $ExpectError
this.state = {
inputValue: 'hello'
};
}
render() { return null; }
}

class BadlyInitializedState extends React.Component<Props, State, Snapshot> {
// ExpectError -> this throws error on TS 2.6
// state = {
// secondz: 0,
// inputValuez: 'hello'
// };
}
class BetterPropsAndStateChecksComponent extends React.Component<Props, State, Snapshot> {
render() { return null; }
componentDidMount() {
// $ExpectError
console.log(this.state.inputValue);
}
mutateState() {
// $ExpectError
this.state = {
inputValue: 'hello'
};

// Even if state is not set, this is allowed by React
this.setState({inputValue: 'hello'});
this.setState((prevState, props) => {
// $ExpectError
props = {foo: 'nope'};
// $ExpectError
props.foo = 'nope';

return { inputValue: prevState.inputValue + ' foo' };
});
}
mutateProps() {
// $ExpectError
this.props = {};
// $ExpectError
this.props = {
key: 42,
ref: "myComponent42",
hello: "world",
foo: 42
};
}
}
}

class ModernComponent extends React.Component<Props, State, Snapshot>
implements MyComponent, React.ChildContextProvider<ChildContext> {
Expand Down Expand Up @@ -682,6 +748,7 @@ React.createFactory(TransitionGroup)({ component: "div" });
// The SyntheticEvent.target.value should be accessible for onChange
// --------------------------------------------------------------------------
class SyntheticEventTargetValue extends React.Component<{}, { value: string }> {
state: { value: string };
constructor(props: {}) {
super(props);
this.state = { value: 'a' };
Expand Down
8 changes: 8 additions & 0 deletions types/react/test/tsx.tsx
Expand Up @@ -141,6 +141,10 @@ class ComponentWithNewLifecycles extends React.Component<NewProps, NewState, { b
return { bar: `${nextProps.foo}bar` };
}

state = {
bar: 'foo'
};

getSnapshotBeforeUpdate(prevProps: Readonly<NewProps>) {
return { baz: `${prevProps.foo}baz` };
}
Expand All @@ -160,6 +164,10 @@ class PureComponentWithNewLifecycles extends React.PureComponent<NewProps, NewSt
return { bar: `${nextProps.foo}bar` };
}

state = {
bar: 'foo'
};

getSnapshotBeforeUpdate(prevProps: Readonly<NewProps>) {
return { baz: `${prevProps.foo}baz` };
}
Expand Down

10 comments on commit 542f3c0

@franklixuefei
Copy link
Contributor

@franklixuefei franklixuefei commented on 542f3c0 Jun 28, 2018

Choose a reason for hiding this comment

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

@Hotell I don't understand. This change breaks a LOT of my code. Why do we have to specify state as an instance variable explicitly now?

@danieloprado
Copy link

@danieloprado danieloprado commented on 542f3c0 Jun 28, 2018

Choose a reason for hiding this comment

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

Now we can't set a initial state!

image

plus, maybe we can use this to make the state full reaonly!

export type DeepReadonly<T> =
  T extends Array<any> ?
  ReadonlyArray<T[0]> :
  T extends Date ?
  T :
  T extends Function ?
  T :
  T extends object ?
  { readonly [P in keyof T]: DeepReadonly<T[P]> } :
  T;

@mjjs
Copy link

@mjjs mjjs commented on 542f3c0 Jun 28, 2018

Choose a reason for hiding this comment

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

Shouldn't such a big change be bumping the major version number? I assume a lot of old code will break due to this change if they have @types/react as ^16.x.x in their package.json.

@franklixuefei
Copy link
Contributor

Choose a reason for hiding this comment

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

@pelotom Tom, if you have time, could you validate this PR and see if specifying state as an instance variable explicitly here is intended? If so, from your knowledge, why?

Thanks a lot!

@mitagg
Copy link

@mitagg mitagg commented on 542f3c0 Jun 28, 2018

Choose a reason for hiding this comment

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

Forcing the state to be readonly is defintelly a breaking change, especially considering the fact that the react documentation is showcasing an initialization in the class constructor.

@twoKilo
Copy link

Choose a reason for hiding this comment

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

it makes more sense that you define a special type in Constructor.

@juliensnz
Copy link

Choose a reason for hiding this comment

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

This change also broke my app so that's a bit annoying. But anyway, do you have an idea how to fix that? I don't feel confident using setState in the constructor as it's explicitly forbidden by the react documentation.

@franklixuefei
Copy link
Contributor

@franklixuefei franklixuefei commented on 542f3c0 Jun 28, 2018 via email

Choose a reason for hiding this comment

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

@juliensnz
Copy link

Choose a reason for hiding this comment

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

Yes, of course, that's what I do, but if I do that, I get this error now:

TS2540: Cannot assign to 'state' because it is a constant or a read-only property.

Here is my code BTW:

constructor(props: TableProps) {
    super(props);

    this.state = {
        nextItemToAddPosition: 0,
    };
}

@juliensnz
Copy link

Choose a reason for hiding this comment

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

Ok, just found the PR with explained workaround: #26813

Please sign in to comment.