Skip to content

react: allow the use of getSnapshotBeforeUpdate#24985

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

react: allow the use of getSnapshotBeforeUpdate#24985
jacobwgillespie wants to merge 3 commits into
DefinitelyTyped: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.


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

Comment thread types/react/test/tsx.tsx
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

typescript-bot commented Apr 14, 2018

Copy link
Copy Markdown
Contributor

@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

jacobwgillespie commented Apr 14, 2018

Copy link
Copy Markdown
Contributor Author

@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