Skip to content

[react]: improve props and state type safety on Component #26813

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

Merged

Conversation

Hotell
Copy link
Contributor

@Hotell Hotell commented Jun 25, 2018

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Changing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: <>

  • Increase the version number in the header if appropriate.

  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

  • This makes props readonly as they are, so consumer will be notified on compile time

  • state needs to be initialised otherwise accessing it or setting it directly will lead to compile error ( which will error on run time as well )

Migration docs

This PR exposes programatic/compile type errors when handling with state. Till know you got a false assumption about using generics for defining state which was not completely correct.

Something like this was allowed before:

type State = { who: string };
class Test extends Component<{}, State> /*(1)*/ {
  render() {
    // no compile-time error
    // runtime error as you cannot access properties on null
    return <div> Who am I {this.state.who} /* (3) */</div>;
  }
  doSomething() {
    // state was not set, this won't throw runtime error though ( react handles that )
    // magic that works...
    // (2)
    this.setState({ who: 'joker' });
  }
}

Which has few issues:

  1. false promise of "my state is defined because it's defined via generics"
  2. calling setState on non initialized state
  3. accessing state for read didn't throw compile time errors, but throw on run-time if it was missing

So overall 👉no explicit state === error prone code

1. migrate to state initialization via class property

Setting state from within constructor manually is not a good practice, especially not when we have transpiler ( TS ), it also forces you to write unnecessary boilerplate:

  • constructor,
  • calling super,
  • annotating constructor args...

for more info check https://medium.freecodecamp.org/react-binding-patterns-5-approaches-for-handling-this-92c651b5af56 - last bullent point 5.)

type Props = {}
type State = { who: string }
class MyCmp extends Component<Props, State> {
-constructor(props:Props){
- super(props)
- this.state = { who: 'Morpheus' }
-}
+readonly state = { who: 'Morpheus' }
}

or (my preference YMMV !) consider extracting initial state to constant, and define type State from implementation 😎👌

type Props = {}
-type State = { who: string }
+type State = typeof initialState
+const initialState = { who: 'Morpheus' }
class MyCmp extends Component<Props, State> {
-constructor(props:Props){
- super(props)
- this.state = { who: 'Morpheus' }
-}
+readonly state = intialState
}

2. I need some state initialization logic, what should I do?

A good practice is to extract any initialization logic to pure function ( which is easy to test + other benefits ). You can define it as a static method if you wish as well.

type Props = {}
type State = { who: string }

+const whoInitLogic = (props:Props) => { ... }
class MyCmp extends Component<Props, State> {
-constructor(props:Props){
- super(props)
- const who = this.superComplicatedWhoInitLogic(this.props)
- this.state = {  who }
-}
+readonly state = { who:  whoInitLogic(this.props) }
}

3. OMG what have you done dude, I don't have time for this, gimme quick fix

Although I highly recommend to spent 2 seconds more and use what is described in 1.), you can do a quick fix by defining state ( to let know Compiler that state is there and it's gonna be initialised within constructor )

type Props = {}
type State = { who: string }
class MyCmp extends Component<Props, State> {
+ state: State
constructor(props:Props){
  super(props)
  this.state = { who: 'Morpheus' }
}
}
  • React "starting" docs use es2015 classes, in TS world we can use class properties which are stage 3 of ECMA standard and will be in next version of ecmaScript + TS transpiles this for us so we are all good :)

@Hotell Hotell requested a review from johnnyreilly as a code owner June 25, 2018 12:04
}

class BadlyInitializedState extends React.Component<Props, State, Snapshot> {
// $ExpectError
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have here the same problem as before. getting dtslint error on TS 2.6

image

should I remove this ?

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Jun 25, 2018
@typescript-bot
Copy link
Contributor

typescript-bot commented Jun 25, 2018

@Hotell Thank you for submitting this PR!

🔔 @jgoz @dmitryrogozhny @eelco @ghotiphud @schwers @michael-yx-wu @willisplummer @smvilar @sulf @incleaf @pepaar @stephenjelfs @ilivit @bolatovumar @ngbrown @theigor @alitaheri @herrmanno @DaIgeb @allienna @schlesingermatthias @InsidersByte @artyomsv @dan-j @minodisk @samwalshnz @johnnyreilly @Methuselah96 @bbenezech @pzavolinsky @digiguru @ericanderson @morcerf @tkrotoff @DovydasNavickas @onigoetz @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @forabi @crohlfs @nicolas-schmitt @pjo256 @robessog @tbayne @cdeutsch @rosskevin @varHarrie @bradleyayers @paustint @screendriver @SupernaviX @KieranPeat @martinnov92 @radziksh @andrewBalekha @smrq @Rogach @royxue @KoalaHuman @uncovertruth @MartynasZilinskas @cameron-mcateer @TiuSh @danielasy @rhysd @m0a @jacobbaskin @jnbt @suniahk @iRoachie @j-fro @gazaret @timwangdev @salim7 @jemmyw @NoHomey @charlesrey @mykter @bsurai @kaoDev @guntherjh @wasd171 @szabolcsx @kraenhansen @Stevearzh @mgoszcz2 @alihammad @mfal @danilobjr @fabiopaiva @FaithForHumans @KurtPreston @timc13 @patrickrgaffney @mthmulders @rapmue @ZheyangSong @richbai90 @caspeco-dan - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@Hotell
Copy link
Contributor Author

Hotell commented Jun 25, 2018

Uff again a lot of errors in 3rd party definitions dependant on this typings, which exposes errors in those ( which is good ! ). Although it's sad to see that react types are not very "strict" and now when we wanna improve things... a lot of refactoring outside is needed 🧐

@typescript-bot
Copy link
Contributor

typescript-bot commented Jun 25, 2018

@Hotell The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@Hotell
Copy link
Contributor Author

Hotell commented Jun 25, 2018

I've updated all react dependant libraries...

props: Readonly<{ children?: ReactNode }> & Readonly<P>;
state: Readonly<S>;
readonly props: Readonly<{ children?: ReactNode }> & Readonly<P>;
readonly state: null | Readonly<S>;
Copy link
Contributor

@ferdaber ferdaber Jun 25, 2018

Choose a reason for hiding this comment

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

Should this instead be:

class Component {
  readonly state?: ReadOnly<S>;
}

React itself does not instantiate state on its own (not even to null), and this more accurately depicts what actually happens (a consumer does not always have to instantiate this.state)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're wrong, state is null by default ✌️

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh! TIL!

Choose a reason for hiding this comment

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

Looks like instantiating state with null creates an issue when one sets state in a constructor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is very incorrect. state shouldn't be union with null.

Copy link
Collaborator

Choose a reason for hiding this comment

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

MAYBE you can give the generic of stats a default value of null. no more than that

Copy link
Contributor

@ferdaber ferdaber left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for contributing, you should consider adding yourself as a maintainer to this package! 😄

@Hotell
Copy link
Contributor Author

Hotell commented Jun 25, 2018

haha I've added myself already here #26545

but sure can do that for this one :)

cheers

@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express and removed Awaiting reviewer feedback labels Jun 25, 2018
@typescript-bot
Copy link
Contributor

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@jgoz
Copy link
Contributor

jgoz commented Jun 25, 2018

The implication of this change is that component classes will need to override their definition of state in order to remove the null type, is that correct?

@Hotell
Copy link
Contributor Author

Hotell commented Jun 25, 2018

The implication of this change is that component classes will need to override their definition of state in order to remove the null type, is that correct?

Yup, which exposes runtime errors during compile. Like accessing this.state.foo when no state was defined ( this was a false assumption till now -> people just defined State via generics, which was wrong as can be seen in lot of updates needed in 3rd party packages )

Also this enforces to follow best practices/design patterns :

  • to always initialise state, if it's used at all ✌️
  • initialise state via class prop instead of creating constructor when it's not needed
  • constructor should/can be used only:
    • when someone uses bind to reassign prototype functions to local props for passing to eventHandlers within JSX. Although cleaner pattern is exposing those functions via class prop
  • to introduce some initialisation logic, although again, preferred pattern is to encapsulate that logic to a pure function that accepts props 👉 state = initState(this.props)

@typescript-bot typescript-bot added Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. and removed Merge:Express labels Jun 25, 2018
@typescript-bot
Copy link
Contributor

@Hotell Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day!

Hotell added 2 commits June 26, 2018 11:06
- fix(react): remove failing expectation on TS 2.6
- chore(react): add myself to contributors
@Hotell Hotell force-pushed the mh/react/better-state-and-props-api branch from 081040c to 98f3e2f Compare June 26, 2018 09:07
@typescript-bot typescript-bot added Merge:Express and removed Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. labels Jun 26, 2018
@paulvanbrenk paulvanbrenk merged commit 0898aaa into DefinitelyTyped:master Jun 26, 2018
@Hotell Hotell deleted the mh/react/better-state-and-props-api branch June 27, 2018 07:39
@tzachshabtay
Copy link
Contributor

@Hotell I found this PR after getting the latest type definitions and getting bombarded with a billion false alerts about state might be null.
I'm not following your explanation on best practices, can you write a small example?
Specifically regarding initialise state via class prop instead of creating constructor when it's not needed: how do I do that?

@Hotell
Copy link
Contributor Author

Hotell commented Jun 28, 2018

@TazmanianD

While I agree that you should generally set the initial state through a property initializer, if the initial component state depends on the component props, that is not possible and you have to do it in the constructor.

That's not entirely correct :)

you can set it from within class property initializer as well, which Is described in migration docs of this PR already :)

class MyComponent extends React.Component<MyComponentProps, MyComponentState> {
   readonly state: MyComponentState = {someValue: this.props.someValue || ''};
}

@TazmanianD
Copy link

@Hotell Thanks for the clarification. I deleted my comment after I realized others had already said what I was saying.

@binki
Copy link
Contributor

binki commented Jun 29, 2018

With this change and the recommended way of creating the property, it is easy to forget the Readonly and write something like this:

export interface IMyComponentState {
  x:string;
}
export default class extends React.Component<{}, IMyComponentState> {
  public readonly state = {
    x: 'hi',
  };
  public componentDidMount() {
    this.state.x = 'yo';
  }

With the constructor initialization style, I was able to be type-safe and avoid this. In fact, I wish TypeScript forbade redefining the property because it effectively allows you to violate patterns which the super class is trying to enforce. Is there any solution to this?

I do like how creating the definition removes the null from the subclass’s property definition. However, at that point you might as well just omit state from the definition of React.Component<> entirely and only have it be defined by the subclass. Why didn’t you go that route?

@Rycochet Rycochet mentioned this pull request Jun 29, 2018
9 tasks
@Rycochet
Copy link
Contributor

Please think more carefully before instantiating some change like this null State again - not only is it breaking for pretty much everyone worldwide, but it is also explicitly against the React documentation.

If you're dead set on keeping it, then might I suggest getting a job on the React team and getting official support for screwing them over?

You cannot fix a simple problem by forcing a complicated partial solution that is not supported (or even referenced) in any official documentation, especially when it has worldwide consequences. There is a reason that most JS packages use SemVer - and this sort of debacle is a prime example of not understanding it at all - https://semver.org/

@rosskevin
Copy link
Contributor

@Rycochet your rebuke is unnecessary, and your comments about semver leads me to believe you have skipped over much of the discussion including the current limitations of this repo's release process.

There were good intentions here and no one was out to inflict pain on others.

@helmutschneider
Copy link

You can say what you want about type safety but this kind of change is unacceptable in a patch version. It shouldn't even be an argument - the old pattern is actively encouraged in official React docs.

@rosskevin
Copy link
Contributor

@helmutschneider everyone agrees. Please read about semver limitations above.

@ericanderson
Copy link
Contributor

Another approach worth considering is using getDerivedStateFromProps and returning your initial state if null.

@ericanderson
Copy link
Contributor

I should note that I think marking state as readonly is the right move but that the null is wrong

@Hotell
Copy link
Contributor Author

Hotell commented Jun 29, 2018

I've submitted PR to revert this #26946

Sorry everyone for trouble that this caused 😥🤒

We should introduce these changes with BC version bump later on

one Pro tip/closing words:

if something breaks, you can always rollback your version of particular package without giving middle fingers and hating on person responsible for that. That won't help anyone and might discourage person who is investing his time for free to make better oss. We are all in this together, so please try to be nice and constructive. Peace ✌️

@Rycochet
Copy link
Contributor

@rosskevin You're right, I was annoyed.

@Hotell I want to apologise for my tone (won't edit above as it was said) - yes, I was somewhat annoyed at the amount of time I had to spend tracking it down, then seeing it was such an annoyingly small change in the midst of a whole load of worthwhile and wanted changes. I'm sorry - and hope you can have a good day despite all the "angry" aimed at you!

So (after eating and thinking) - I think that this (null State, readonly is correct) is a change that should get in though not necessarily in this form, but going back to the whole semver discussion - it should be queued up for the next React major change - and should get some involvement from them so that it actually gets supported in the example code.

Having said that, it feels like something that needs thinking about in the best way to do it. Having an initialiser property doesn't help unless you have static values (even if it's via function call), and having to state the type again to override the null doesn't actually force you to initialise it in the constructor, it's just like putting an "as any" on the end of something because you can't figure out a better way to do it (well it does say it's a quick fix!).

I'm wondering if anyone can think of a better way for it to be done - I generally keep up with the TS updates, but only have my eye on about a dozen open TS issues, and none of them are relevant...

@bbenezech
Copy link
Contributor

Please people:

On this specific issue

  • React docs are written in es5 JS, not TS. Expect patterns in React docs that have no static representation ATM and have better alternative in TS (i.e. this or the whole currentTarget vs target madness). We can and should push for more type safety on top of React best practices, otherwise why are we here?
  • The added type safety is worth it. I've seen plenty of this.state.whatever fail in the wild. If this can be caught statically, it should be.
  • Using a constructor was a weird idea from day one https://hackernoon.com/the-constructor-is-dead-long-live-the-constructor-c10871bea599. It's error-prone, useless and verbose. If you did it on 500+ components, you must have a good reason. You can of course cargo your way around with a few components, but on a codebase that big... https://www.youtube.com/watch?v=oAKG-kbKeIo

About Semver (AFAIK, I'm not a DT maintainer)

  • There is no Semver on DT. Don't expect it.
  • Versions are inferred from implementation.
  • The https://blog.johnnyreilly.com/2017/02/typescript-types-and-repeatable-builds.html link is outdated and erroneous, lock your dependencies (yarn.lock). Even "fixing to specific definitions" won't work with sub-dependencies.
  • "Breaking changes" in typings don't actually break much since your users will never suffer from it directly. Keeping that word for functional bugs and regressions clears the field.
  • Please do not complain for a change requiring refactoring on your side. A big codebase is a big responsibility. If you are wrong, fork the dependency, use a codemod or an intern, whatever works. If the dependency upstream is wrong, PR it. Unsure ? Ask.

@Rycochet
Copy link
Contributor

@bbenezech One thing to point out on your comment there - Typescript should be able to take Javascript and (provided that good practices are in order) should have no problems with it. That one specific change broke every single piece of React example code that dealt with State and forced extra code to handle it that's not part of the React docs. The change needs to come from that end, or at the very least be kept on the sidelines till React has a major version bump so it's not breaking anyone who uses the default package.json rules. Better code is good, but accidentally breaking close to 100% (estimated) of existing React code with no way to know how to fix it without being able to track things down is not a good way to promote change.

Unfortunately your link to hackermoon loses a lot of its influence when it's got such bad code examples as this:

class Foo extends Component {
  myHandler = this.myHandler.bind(this);
  myHandler() {
    // some code here that references this
  }
  ...
}

Instead of this:

class Foo extends Component {
  myHandler = () => {
    // some code here that references this
  }
  ...
}

Bind is equally dead (unless you have a valid use for it, and that isn't one).

It also refers to componentWillMount - so really shouldn't be used as an example of anything really :-/

@efc
Copy link

efc commented Jun 29, 2018

As someone quite new to this community with only a few weeks of React and TypeScript experience, I want to say that this whole discussion is quite an eye-opener. My partner and I were completely baffled by why our code suddenly stopped working. We spent two days hunting through our package.json and webpack settings blaming ourselves and our thin veneer of "knowledge" of these tools for the failure. By the time we had tracked the problem down to this @types/react change yesterday, a very helpful Stack Overflow questions had been asked and answered pointing to this thread.

While I appreciate the type-safety this change was meant to encourage (and I especially appreciate @Hotell's examples of best practice patterns above), I am very glad to see it being reverted in #26946. I fear there are many other folks in a boat similar to ours, still pulling their hair out wondering what they did wrong and why none of the React examples work any longer. The sooner a version of @types/react supports the predominant (and only documented) practice, the better.

For myself, I wonder if anyone here has advice about how to best follow the development of @types/react so that we would have at least been warned of this breaking change. Is there a feed of release notes or some such tool that we should be keeping an eye on?

@gilbsgilbs
Copy link
Contributor

gilbsgilbs commented Jun 29, 2018

Hi,
I came here because this PR broke my project. While I understand the rationale behind it (provided that it comes with a major version bump, as other folks have already mentioned), I have some questions and remarks:

  • Is the migration process documented or going to be documented somewhere (apart from this PR)? I mean, it's already quite hard to setup a basic React project with TypeScript provided how this repo lacks proper docs. If it starts BC without giving any clue on how to upgrade, it will really be painful for users in the long run.
  • Is there any reason to keep the State generic then? I find it to be redundant and error-prone with your change, but maybe it's necessary (in setState function for instance).
  • Isn't it a bit rude to force people into initializing their state outside the constructor although it's a valid and documented use-case (documented in React's doc, which at very least exists)? I also find it better, but it seems to be a matter of taste, not that it's really, inherently better. Moreover, in some cases, it might even be sub-optimal.

@alamothe
Copy link

alamothe commented Jun 30, 2018

@Hotell Do you find this to be a major issue with "state as a property" approach:

One can completely forget to add State as a generic parameter. At that point, passing anything into setState compiles:

type State = {
	dismissed: boolean
}

// We forgot to pass State as a superclass generic parameter
export class Alert extends React.Component<Props> {
	
	public readonly state: State = {
		dismissed: false,
	}

	private onClick = () => {
		// Anything goes!
		this.setState({
			whatever: 123,
		})

Maybe this can be fixed by changing the default for State generic parameter?

Interested in hearing your thoughts

JulianG added a commit to JulianG/bananatabs that referenced this pull request Jun 30, 2018
JulianG added a commit to JulianG/react-list-drag-and-drop that referenced this pull request Jul 2, 2018
@jwbay jwbay mentioned this pull request Dec 7, 2018
9 tasks
alamenai added a commit to alamenai/react-1 that referenced this pull request Jan 6, 2023
Based on this [PR](DefinitelyTyped/DefinitelyTyped#26813) we don't have to put `readonly` for our states, because `React.Component<P,S>` already marks them as immutable.
@@ -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>);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will remove the Readonly wrapper in #69140 since it doesn't seem to do anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Owner Approved A listed owner of this package signed off on the pull request. Popular package This PR affects a popular package (as counted by NPM download counts).
Projects
None yet
Development

Successfully merging this pull request may close these issues.