Add definitions for React 16.6's React.memo by Jessidhia · Pull Request #29990 · DefinitelyTyped/DefinitelyTyped

Jessidhia · 2018-10-24T02:18:06Z

CI finally passes 🎉

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).
- It's broken locally? Error: Could not parse version: line is '// TypeScript Version: 2.8'
  - What is going on I have to abuse CI for tests because I can't run any testing locally
- Fixed with: rm -rf node_modules types/*/node_modules && git clean -f -d

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: https://reactjs.org/blog/2018/10/23/react-v-16-6.html#reactmemo
- I don't know if it's intended, but it technically works with class components, so I included support for it in the types.
Increase the version number in the header if appropriate.

typescript-bot · 2018-10-24T02:22:26Z

@Kovensky Thank you for submitting this PR!

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

plantain-00 · 2018-10-24T03:13:22Z

@Kovensky Can you add static contextType in this PR? it's introduced in 16.6 too

Jessidhia · 2018-10-24T03:41:11Z

@plantain-00 I actually wrote an issue about that because TypeScript doesn't support generics on the constructor type, which would be required for this.context to correctly get its type from static contextType: #29989

Jessidhia · 2018-10-24T03:46:18Z

I assume the failures with TypeScript@next are related to microsoft/TypeScript#27627

EDIT: here's the bug: microsoft/TypeScript#27948

typescript-bot · 2018-10-24T04:22:26Z

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

Jessidhia · 2018-10-24T04:24:19Z

I fixed things enough such that the tests for react itself now pass. However, other libraries that are making use of React.ComponentType will have a hard time adapting to TS@next's new overload resolution...... it's not something that can be fixed in react, has to be either fixed in TS@next or on the users themselves.

typescript-bot · 2018-10-24T05:22:25Z

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

vagusX · 2018-10-24T08:44:55Z

@Kovensky would you like to add static getDerivedStateFromError in this PR?

    interface StaticLifecycle<P, S> {
        getDerivedStateFromProps?: GetDerivedStateFromProps<P, S>;
        getDerivedStateFromError?: GetDerivedStateFromError<S>;
    }

    type GetDerivedStateFromError<S> =
        /**
         * Receiving the error that was thrown as a parameter  and returns an update to a component's state.
         * This lifecycle is invoked after an error has been thrown by a descendant component.
         */
        (error: Error) => Partial<S> | null;

BTW, react 16.6 also includes some other features, would you implement all? Maybe I could help it out too.

ferdaber · 2018-10-24T13:33:18Z

+    //
+    // We don't just use ComponentType or SFC types because you are not supposed to attach statics to this
+    // object, but rather to the original function.
+    interface ExoticComponent<P = {}> {


If we're introducing the concept of exotic components (components without statics, essentially), would it be a good idea to introduce ExoticComponent as one of the union types in React.ComponentType? To userland I believeComponentType and ReactType are all the "all-encompassing" types that can be used as JSX value-based components.

Never mind, I just read up on microsoft/TypeScript#27948.

ExoticComponent does happen to be assignable to SFC so it does work as-is.

I wonder what I can do about memo and forwardRef, though... I could have a sub-interface with an extra type prop for memo, and extend LibraryManagedAttributes to check there.

As for forwardRef, it already does not support LibraryManagedTypes; I tried to look into ReactFiberBeginWork's code to see if it supports defaultProps, but the code path that does access defaultProps seems to be unreachable according to my limited understanding of Fiber. At any rate, even if it is reachable, it does happen to be looked up as a static added to the exotic component itself, so that'd have to be another subtype I guess.

If it's a static added to the exotic component itself, wouldn't it defeat the purpose of it being an exotic component, since that type was created to discourage adding statics to it? I guess it's a smaller subset of the statics allowed in vanilla components.

Only forwardRef has that, though, not memo. They're more like subtypes of ExoticComponent.

ferdaber · 2018-10-24T13:40:14Z

+         *
+         * @deprecated Exotic components are not callable
+         */
+        (props: P): (ReactElement<any>|null);


Aside: I'm surprised that this actually works with how the compiler resolves JSX value-based elements. I had thought that the return type of this call signature needs to be assignable to JSX.ElementClass, but I guess if the tests are passing, then it still works?

This looks like an SFC signature to the compiler, so it's return value is only checked against JSX.Element | null.

ferdaber · 2018-10-24T13:41:42Z

+                ? P
+                : {};
+
+    function memo<T extends ComponentType<any>>(


Both memo and forwardRef will break the LibraryManagedAttributes inference, since the static properties are not copied over. Is there any way we can preserve those when using these "HOC-like" functions?

I tried to in my follow-up PR, but it seems LibraryManagedAttributes just don't work at all with non-class components, even if the type that JSX.LibraryManagedAttribute extracts is correct.

Is this still the case? AFAIK it should apply correctly to all components now.

ferdaber

Looks good to me aside from the issues with the TSnext's new changes re: multiple call signatures for JSX tags.

Jessidhia · 2018-10-25T04:42:28Z

Yeah, the call signature thing will either be fixed on the TypeScript side, or those individual dependencies will have to all be updated and for them it'll likely be a breaking change.

I'll see if I can get LibraryManagedProps working with memo and forwardRef in a follow up PR to not block merging this one; same for adding the other new types.

I still don't know what to do with the new static variables and lifecycles though; it's not possible in TS to use generics in the constructor type itself. See #29989

eljenso · 2018-10-25T13:02:30Z

@Kovensky Would you consider including lazy and Suspense into this PR? See #30031

Hotell · 2018-10-25T13:07:49Z

https://twitter.com/martin_hotell/status/1055381631094190080

eljenso · 2018-10-25T13:45:39Z

@Hotell So we'll have to wait for TS 3.2 for these types to be merged?

Hotell · 2018-10-25T13:49:03Z

"unfortunately" yes I think so @eljenso

Jessidhia · 2018-10-25T14:32:15Z

I'm not sure why. The tests are already broken on master if react is tested at all. This PR actually reduces the breakage.

Blocked by enzymejs/enzyme#1875 and DefinitelyTyped/DefinitelyTyped#29990

typescript-bot · 2018-11-02T02:26:11Z

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

TimoWestland · 2018-11-05T13:05:37Z

Any updates on this? :)

vishnutsivan · 2018-11-07T07:07:13Z

Waiting for the merge :(

johnnyreilly · 2018-11-07T07:38:21Z

We can't merge until we've a passing build I'm afraid. No doubt this will be solved but this is open source; people are doing this for ♥️ in their spare time. Please bear with them until they can.

typescript-bot · 2018-11-08T03:25:50Z

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

TimoWestland · 2018-11-08T16:09:22Z

Completely understand! Not trying to push, was just wondering about the status on this because all the todos were done so it wasn’t clear if there was any work left to do to get the build working. Thanks for the update! I’ll just keep watching this thread.

Jessidhia · 2018-11-09T00:20:49Z

Looks like a simple text fix remains, I'll do it once I'm at my computer 🎉

I think I'll also tone down a bit on the warning for the call signature, it annoyingly shows as if it was documentation even when the component is being used in JSX...

typescript-bot · 2018-11-09T06:30:50Z

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

Jessidhia · 2018-11-09T06:44:23Z

The other PR (#30054) which included all commits from this one too was merged 🎉

donaldpipowitch · 2018-11-09T06:49:38Z

@Kovensky Thank you very much!

DanielRosenwasser · 2018-11-09T20:05:06Z

@Kovensky just wanted to say thanks for all the awesome work you've done here on the React declarations on DefinitelyTyped! The last few weeks have likely been tiresome so I'm sure we all appreciate it.

zaguiini · 2018-11-09T20:06:49Z

If I lived near @Kovensky, I'd certainly pay her a beer!

TimoWestland · 2018-11-10T11:01:15Z

This is aweomse, @Kovensky. Thank you :)

Hotell · 2018-11-19T04:10:51Z

arigato gozaimasu @Kovensky ❤️

Jessidhia requested a review from johnnyreilly as a code owner October 24, 2018 02:18

typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Oct 24, 2018

typescript-bot added The Travis CI build failed and removed Awaiting reviewer feedback labels Oct 24, 2018

Jessidhia mentioned this pull request Oct 24, 2018

TSX and React Context API broke on nightly microsoft/TypeScript#27948

Closed

Jessidhia mentioned this pull request Oct 24, 2018

Is it intended that React.memo() also works with class components? facebook/react#13937

Closed

MichalLytek approved these changes Oct 24, 2018

View reviewed changes

typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label Oct 24, 2018

ferdaber reviewed Oct 24, 2018

View reviewed changes

ferdaber approved these changes Oct 24, 2018

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Oct 24, 2018

lgeiger added a commit to lgeiger/nteract that referenced this pull request Oct 25, 2018

Modernize ansi-to-react and try out React.memo

ebe8ddb

Blocked by enzymejs/enzyme#1875 and DefinitelyTyped/DefinitelyTyped#29990

lgeiger mentioned this pull request Oct 25, 2018

Modernize ansi-to-react and try out React.memo nteract/archived-desktop-app#3524

Closed

This was referenced Oct 26, 2018

[@types/react] Add properties Suspense & lazy #30031

Closed

Add more of the new features added in React 16.6 #30054

Merged

bdwenxi mentioned this pull request Nov 1, 2018

Property 'memo' doest not exist on type 'typeof React'? facebook/react#14059

Closed

typescript-bot added Merge:Express and removed The Travis CI build failed labels Nov 5, 2018

Jessidhia force-pushed the react-16-memo branch from 0b45f71 to 5bd3fe2 Compare November 8, 2018 02:36

weswigham closed this Nov 8, 2018

weswigham reopened this Nov 8, 2018

Jessidhia force-pushed the react-16-memo branch from 5bd3fe2 to 4d14cf4 Compare November 9, 2018 01:56

typescript-bot added Merge:Express Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. and removed The Travis CI build failed labels Nov 9, 2018

Jessidhia closed this Nov 9, 2018

Jessidhia force-pushed the react-16-memo branch from 4d14cf4 to 9039892 Compare November 9, 2018 06:43

Jessidhia deleted the react-16-memo branch November 9, 2018 06:44

Conversation

Jessidhia commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plantain-00 commented Oct 24, 2018

Uh oh!

Jessidhia commented Oct 24, 2018

Uh oh!

Jessidhia commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Oct 24, 2018

Uh oh!

Jessidhia commented Oct 24, 2018

Uh oh!

typescript-bot commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vagusX commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jessidhia Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ferdaber left a comment

Choose a reason for hiding this comment

Uh oh!

Jessidhia commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eljenso commented Oct 25, 2018

Uh oh!

Hotell commented Oct 25, 2018

Uh oh!

eljenso commented Oct 25, 2018

Uh oh!

Hotell commented Oct 25, 2018

Uh oh!

Jessidhia commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimoWestland commented Nov 5, 2018

Uh oh!

vishnutsivan commented Nov 7, 2018

Uh oh!

johnnyreilly commented Nov 7, 2018

Uh oh!

typescript-bot commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimoWestland commented Nov 8, 2018

Uh oh!

Jessidhia commented Nov 9, 2018

Uh oh!

typescript-bot commented Nov 9, 2018

Jessidhia commented Oct 24, 2018 •

edited

Loading

typescript-bot commented Oct 24, 2018 •

edited

Loading

Jessidhia commented Oct 24, 2018 •

edited

Loading

typescript-bot commented Oct 24, 2018 •

edited

Loading

vagusX commented Oct 24, 2018 •

edited

Loading

Jessidhia Oct 24, 2018 •

edited

Loading

Jessidhia commented Oct 25, 2018 •

edited

Loading

Jessidhia commented Oct 25, 2018 •

edited

Loading

typescript-bot commented Nov 2, 2018 •

edited

Loading

typescript-bot commented Nov 8, 2018 •

edited

Loading

DanielRosenwasser commented Nov 9, 2018 •

edited

Loading