Skip to content

WIP: [react] more strict typings with breaking changes#26956

Closed
Hotell wants to merge 4 commits intoDefinitelyTyped:masterfrom
Hotell:mh/react/more-strict-typings-with-bc
Closed

WIP: [react] more strict typings with breaking changes#26956
Hotell wants to merge 4 commits intoDefinitelyTyped:masterfrom
Hotell:mh/react/more-strict-typings-with-bc

Conversation

@Hotell
Copy link
Copy Markdown
Contributor

@Hotell Hotell commented Jun 29, 2018 •
edited
Loading

This reintroduces changes from #26813 and adds other type safety improvements

NOTE:

Breaking Changes:

  1. if your component supports children prop you need to explicitly define it in your component props definition

Before:

class TestChildrenClass extends React.Component {
  render() {
    return this.props.children;
  }
}
const App = () => (
  <div>
    <TestChildrenClass />
  </div>
);

After:

class TestChildrenClass extends React.Component<{ children: React.ReactNode }> {
  render() {
    return this.props.children;
  }
}

// Error, children is required
const App = () => (
  <div>
    <TestChildrenClass />
  </div>
);

// no errors
const App = () => (
  <div>
    <TestChildrenClass> Im allowed and type child! </TestChildrenClass>
  </div>
);
  1. state is defined as Readonly<S> | null union type. In runtime if state is not set it's null by default. You need to set state via class property or if you're initializing it from constructor explicitly provide typescript property definition, to tell the compiler that you're gonna use state which will be defined within constructor instead of property.

Update:
I reverted marking state as readonly -> based on this discussion

Before:

type Props = {};
type State = { who: string };
class MyCmp extends Component<Props, State> {
  constructor(props: Props) {
    super(props);
    this.state = { who: 'Morpheus' };
  }
  render() {
    return null;
  }
}

// following runtime errors ( forgetting to set state ) were not exposed during compile time

class MyCmp extends Component<Props, State> {
  constructor(props: Props) {
    super(props);
  }
  render() {
    return this.state.who
  }
}

After:

type Props = {};
type State = { who: string };
class MyCmp extends Component<Props, State> {
  // you need to explicitly define state, to tell TS that it will be defined within constructor - idiomatic TS
  readonly state: State;
  constructor(props: Props) {
    super(props);
    this.state = { who: 'Morpheus' };
  }
}

// OR set state via class property and completely skip constructor

type Props = {};
type State = { who: string };
class MyCmp extends Component<Props, State> {
  readonly state = { who: 'Morpheus' };
}

// following will produce compile error now, as state is `null` if not set, which is correct behaviour reflecting runtime in compile time

class MyCmp extends Component<Props, State> {
  render() {
    // $ExpectError -> this.state is null or S -> strictNullCheck errors
    return this.state.who
  }
}

  1. all TS strict flags are now enabled 🖖

  2. Component,PureComponent are now abstract so you have to implement always render which will mitigate any previous runtime errors on compile time

Before:

// this compiles without errors
class Test extends Component {}

After:

// this throws compile error as Component is abstract and you need to implement render
class Test extends Component {}
  1. change generics order to get better type inference
  • type CElement<T extends Component, P = T['props']> = ComponentElement<T, P>;
  • interface ComponentElement<T extends Component, P = T['props']> extends ReactElement<P> {
  • others, pls see diff
  1. Props generic has now default value object which describes runtime ( if you won't provide props, it will be empty object by default ) {} is like mixed in Flow ( which allows to set primitives and non-primitive values as well

Before:

// this compiles without errors
class Test extends Component<number> {}

After:

// this throws compile error as Component props need to be `object`
class Test extends Component<number> {}
  • 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).

If changing an existing 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" }.

Hotell added 2 commits June 29, 2018 19:46
@Hotell
feat(react): make react typings better by introducing breaking changes
e8b9ed3 
Breaking Changes:

- if your component supports children prop you need to explicitly define it in your component definition
- state is null by default also marked as readonly, you need to set state via class property or if you're initializing it from constructor explicitly provide typescript prop definition
- all strict flags are now enabled
- Component,PureComponent are now abstract so you have to implement always render which will mitigate any previous runtime errors on compile time
@Hotell
feat(react-dom, react-addonst-test-utils): update definitions to refl…
75b5a5d 
…ect react definition changes
@Hotell Hotell requested a review from johnnyreilly as a code owner June 29, 2018 18:12
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Jun 29, 2018
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Jun 29, 2018 •
edited
Loading

@Hotell Thank you for submitting this PR!

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

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Jun 29, 2018 •
edited
Loading

@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!

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Jun 29, 2018 •
edited
Loading

@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!

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Jul 5, 2018

@Hotell I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues.

@Jessidhia
Copy link
Copy Markdown
Member

Jessidhia commented Jul 6, 2018

Bad bot 😞

@typescript-bot typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Jul 6, 2018
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Jul 6, 2018

@Hotell To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you!

@Hotell
Copy link
Copy Markdown
Contributor Author

Hotell commented Jul 8, 2018

Hmm so no interest in making things better I see. I guess community/Microsoft is too afraid of pushing things forward/getting better type-safety with React apps. Overall I'm little sad about this. but yeah it's open source === no guarantees :) Just to mention for anyone who will read this, breaking changes will be necessary sooner than later to make defaultProps work and also to apply namespaced jsx factory to prevent global jsx clashes. Hopefully someone will carry on and leverages changes from this unanswered PR. 👋

@Nimelrian
Copy link
Copy Markdown

Nimelrian commented Jul 9, 2018

Hmm so no interest in making things better I see. I guess community/Microsoft is too afraid of pushing things forward/getting better type-safety with React apps.

It would have been wise to ping the authors of the library typings which started failing due to this breaking change and coordinate with them, instead of just letting the failed CI build sit there until the bot closes the PR.

You can't expect all the authors to regularly check for PRs to the main react package.

@Hotell
Copy link
Copy Markdown
Contributor Author

Hotell commented Jul 9, 2018

They have been pinged automatically :)

image

  • no discussion happened here
  • if you know a way how to turn the bot off that might help :)

@Nimelrian
Copy link
Copy Markdown

Nimelrian commented Jul 9, 2018 •
edited
Loading

It pinged the react typing maintainers. Not the maintainers of the typings of the other dozen or so libraries you broke with this change.

You may want to check the CI log and see what your change broke in other typings...

@Hotell Hotell mentioned this pull request Jul 13, 2018
Closed
@goloveychuk
Copy link
Copy Markdown

goloveychuk commented Jul 19, 2018 •
edited
Loading

@Hotell
I want to sugest one more change for more strictness. So currently we have incorrect variance check in callbacks in react components, if callback is defined with method syntax.
See this microsoft/TypeScript#25451

interface Props {
  a(b: string, c: string | number): number;  // see c argument
  b: {
      c(b: string, c: string | number): number;
  }
}

type StrictifyProps<T> = {
  [K in keyof T]:
      T[K] extends (...args: infer A) => infer R
        ? (...a: A) => R                     // making property from method
        : T[K] extends Object         //go deeply
             ? StrictifyProps<T[K]> 
             : T[K]
};


const a = (b: string, c: string) => 42; // see c argument

type P = StrictifyProps<Props>;

let x!: P;
// x.
let c: P["a"] = a; //now has error

let b: P['b'] = {
    c: a //now has error
}

The code above fixes this problem. All is needed is to add this to react typings, where we have props: P
Or make private _publicProps: StrictifyProps<P> and
interface ElementAttributesProperty { _publicProps: {}; }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johnnyreilly johnnyreilly Awaiting requested review from johnnyreilly johnnyreilly is a code owner

Assignees

No one assigned

Labels

Abandoned This PR had no activity for a long time, and is considered abandoned Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Hotell @typescript-bot @Jessidhia @Nimelrian @goloveychuk