Skip to content

react: allow the use of getSnapshotBeforeUpdate#24985

Closed
jacobwgillespie wants to merge 3 commits intoDefinitelyTyped:masterfrom
jacobwgillespie:fix-react-getsnapshotbeforeupdate
Closed

react: allow the use of getSnapshotBeforeUpdate#24985
jacobwgillespie wants to merge 3 commits intoDefinitelyTyped:masterfrom
jacobwgillespie:fix-react-getsnapshotbeforeupdate

Conversation

@jacobwgillespie
Copy link
Copy Markdown
Contributor

In the react types, the definition of JSX.ElementClass is extends React.Component<any>. This means that it overrides the default generic for props P to be any, but it accepts the default values for the other two generics, state S and the new snapshot SS. This means that it assumes all ElementClass instances are:

  • Props P: any
  • State S: {}
  • Snapshot SS: never

This causes an issue that prevents using any components that use the new getSnapshotBeforeUpdate lifecycle method, because this forces it to only accept the value of never | null as the snapshot type.

This PR updates the definition of JSX.ElementClass to be extends React.Component<any, {}, any>, which will then accept the following types:

  • Props P: any
  • State S: {}
  • Snapshot SS: any

This makes it possible to use getSnapshotBeforeUpdate.

The first commit in this PR adds a failing test. The second commit makes the definition change.

Fixes #24820. See #24820 for more context.


  • 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: https://reactjs.org/docs/react-component.html#getsnapshotbeforeupdate
  • 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" }.

@jacobwgillespie
Copy link
Copy Markdown
Contributor Author

Well, I'm not sure why the initial test commit isn't failing on Travis CI - locally if I run npm test on 4fa0c20, it fails the React test.

return this.state.bar;
}
}
<ComponentWithNewLifecycles foo="bar" />;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does it also work with React.createElement?

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Apr 14, 2018
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 14, 2018

@jacobwgillespie Thank you for submitting this PR!

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

@jacobwgillespie
Copy link
Copy Markdown
Contributor Author

jacobwgillespie commented Apr 14, 2018

@Kovensky, so while fixing the examples for React.createElement, I discovered a lot of other places that also experience this issue. I just pushed a fix so that this works with both React tests, however that change spidered to react-dom, react-addon-test-utils, and others.

I think it might not be possible to have SS for the snapshot default to never, as any third-party library that did not define something for the third generic parameter of React.Component would be incompatible with any component that used a snapshot. There are over 7000 matches for React.Component< in this repo, so I think we might have to change the value of SS to be something that will tolerate not being specified.

The only thing I know of would be to change React.Component snapshot to be any:

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

Is that best? I know TypeScript 2.8 has the ability to infer the return type of a function, is there some way to use that here so that the type doesn't have to be specified on the Component interface?

@jacobwgillespie
Copy link
Copy Markdown
Contributor Author

Yeah, the tests failed here as react-redux isn't compatible with SS = never

@typescript-bot
Copy link
Copy Markdown
Contributor

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

@jacobwgillespie
Copy link
Copy Markdown
Contributor Author

I have opened #24987 to replace this PR - closing.

@jacobwgillespie jacobwgillespie deleted the fix-react-getsnapshotbeforeupdate branch April 14, 2018 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

3 participants