you didn't read what I've writen did you ? :) Please try again #26813 (comment)

TL;DR:

This change is no rollback at all :) If you wanna rollback, you need to remove union with null as well.

You should not be rude - it will not help.
I propose to rollback the main reason of all controversy. Union with null is a valid change that correspond to the way react works.
P.S.
Usually the developer that breaks code is responsible for rollback. You are free to follow this advise if your wish.

I'm not rude mister :) just annoyed to repeat again things that I've explicitly described and pointed to.

Please remove the null as explained. And you're speaking against yourself, that null is a valid change, it's not as it will introduce "breaking change" as well

@magnetnation Thank you for submitting this PR!

🔔 @johnnyreilly @bbenezech @pzavolinsky @digiguru @ericanderson @morcerf @tkrotoff @DovydasNavickas @onigoetz @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa - 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.

@magnetnation One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

Maybe I missing something but react state default value is null if not initialized and therefore the declaration of state should look like:

state: null | Readonly<S>;

Exactly the way you did it in the original commit. So I suggest to leave it.

Regarding the errors that might arise from the union with null they are on the mercy of each development team and can be switched on/off using strictNullChecks flag of tsc.

Regarding the errors that might arise from the union with null they are on the mercy of each development team and can be switched on/off using strictNullChecks flag of tsc.

yeah that's right ✌️ thanks again and sorry for casting "rude" behaviour on you, that wasn't my intention at all

Are there plans to push a new package with this fix?

Having this null union is a giant pain in our codebase right now...

@milesj feel free to submit a PR to improve things! Being able to initialise state in the constructor is a must but otherwise go forth and conquer!

having the null breaks react-redux connect(). please don't do this in a patch update.

hmm so I double checked again today, and this won't fix introduced breaking change so we have to rollback completely. But I told you so! @magnetnation :D #26912 (comment) :(

going to submit revert PR asap.

sorry everyone for trouble that this caused.

We should introduce these changes with BC version after

one Pro tip though:
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 ✌️

I might be not so bad idea.

I would like to underline one thing. The goal of typescript definitions is not to enforce best practices or make it easier to use it in other libraries. Instead it is to describe underlying react behavior as exact as possible (same idea here).

Lets not forget this.

And if you are going to rollback all changes - please do not forget to reintroduce state: null | Readonly<S> with corresponding version bump.

@magnetnation

please do not forget to reintroduce state: null | Readonly<S> with corresponding version bump.

I think we cannot do that, as it would "prevent" users to use "react documentation way" to set state from within constructor. Hmm actually it would be still possible but they would need to define the class property type - as they should anyway - idiomatic Typescript.

There's also other ways to set state during the constructor phase, that isn't written in the constructor explicitly. A few contrived examples:

A helper method.

class Foo extends React.Component {
  constructor(props) {
    super(props);

    this.setupInitialState();
  }

  setupInitialState() {
    // Lots of logic
    this.state = state;
  }
}

Or state being derived via event/subscription/etc. FinalForm works this way.

class Foo extends React.Component {
  constructor(props) {
    super(props);

    this.api = new API(this.handleState);
  }

  handleState(state) {
    // Lots of logic
    this.state = state;
  }
}

Not sure if TS is smart enough to catch these.

A helper method.

best practice - you should prefer extracting initializers to pure functions

Or state being derived via event/subscription/etc. FinalForm works this way.

that's possible, but you have to define state upfront

class Foo extends React.Component {
+  state: State
  constructor(props) {
    super(props);

    this.api = new API(this.handleState);
  }

  handleState(state) {
    // Lots of logic
    this.state = state;
  }
}

Anyways

good/best practice is to always initialize state ( same like with redux pattern )

Setting the state type property requires you to define every field in the state, with a default value, which you're already doing in the helper method. You're basically duplicating your code, and increasing your file size, especially if there's a lot of fields in your state.

Also, has the readonly/null changes been verified with getDerivedStateFromProps? I didn't see a test case for it.

You're basically duplicating your code, and increasing your file size, especially if there's a lot of fields in your state.

What are you talking about ? you'll have one pure function const initializeState = (props:Props):State => {...}

Also, has the readonly/null changes been verified with getDerivedStateFromProps? I didn't see a test case for it.

instance definitions have no effect on static properties/methods ✌️

It was in reference to the 2nd example. You can't state: State without setting the property value.