@raon0211 Thank you for submitting this PR!

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

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

It seems that other pull requests are currently also failing because of timeout (#33462 #33548 etc).

This is a bug with FunctionComponent, not with ExoticComponent. It's only kept like that on FunctionComponent for backwards compatibility.

I got it. But then I think the type definition below should be fixed to return NamedExoticComponent<PropsWithChildren<P>> for consistency, maybe?

function memo<P extends object>(
    Component: SFC<P>,
    propsAreEqual?: (prevProps: Readonly<PropsWithChildren<P>>, nextProps: Readonly<PropsWithChildren<P>>) => boolean
): NamedExoticComponent<P>;

This will only affect the behavior of memo when FunctionComponent is passed as an argument, and will fix the somewhat confusing behavior addressed at the top.

I'm also running in this issue when trying to pass children to a memoized component.

Is there a known workaround?

This issue also effects usage of forwardRef:

import * as React from 'react';

type Props = {};
declare class C extends React.Component<Props> {}

type ComponentInstance = InstanceType<typeof C>;
const ForwardedRef = React.forwardRef<ComponentInstance, Props>(
    (props, ref) => <C {...props} ref={ref} />,
);

/* Type '{ children: string; }' has no properties in common with type 'IntrinsicAttributes & RefAttributes<C>' */
<ForwardedRef>foo</ForwardedRef>;

@raon0211 Would you mind also adding a failing test for this?

I'm able to workaround the issue with memo with a cast:

import * as React from 'react';

type Props = {};
declare class C extends React.Component<Props> {}

const Memoized = React.memo(C) as React.ComponentType<Props>;

<Memoized>foo</Memoized>;

However I have no such workaround for forwardRef.

The memoized component's props interface should have children set explicitly. Relying on the automatic injection of children is not sound, which is why we don't want to add this change in.

I agree with that it is better to set children in Props, and the implicit injection of children doesn't seem good. However it seems that the inconsistency in the behavior of the original component and the components with memo and forwardRef seems more awkward to me.

The type definition of memo below also is confusing, why specify PropsWithChildren in propsAreEqual but not in the resulting ExoticComponent?

function memo<P extends object>(
    Component: SFC<P>,
    propsAreEqual?: (prevProps: Readonly<PropsWithChildren<P>>, nextProps: Readonly<PropsWithChildren<P>>) => boolean
): NamedExoticComponent<P>; // Why not PropsWithChildren<P> here?

@ferdaber Thanks for your help here. Can you clarify whether you're suggesting to make other (different) changes to the typings in order to fix this, or whether this is a "won't fix" and is instead for users to fix by explicitly specifying children props?

@OliverJAsh You may modify the current type definition by declaration merging to fix the problem temporarily. For example I am using the following definition file in my project to fix the problem for memo:

declare namespace React {
  function memo<Props extends object>(
    Component: SFC<Props>,
    propsAreEqual?: (
      prevProps: Readonly<PropsWithChildren<Props>>,
      nextProps: Readonly<PropsWithChildren<Props>>
    ) => boolean
  ): NamedExoticComponent<PropsWithChildren<Props>>;
}

I will add the test for forwardRef shortly after the discussion, if we decide to edit the type definition.

@OliverJAsh I am mostly advocating for the latter: i.e. userland should be explicitly specifying children in props instead of relying on a different change in the typings.

@ferdaber So it may be that the inconsistency is intended to make users specify children in props?

Kind of... it's more that we just don't want to add to the cruft. Removing the injected children right now will cause lots of breaking changes, and this package (like everything in DT) does not follow semver so that's not a good idea. We also don't want to inject here too so that it's even more breaking changes to have to go through when we eventually update everything.

@ferdaber Thanks for explaining :-) May I ask why we want to avoid injecting children? Why is this not sound?

I understood. Thanks for your kind explanation, I should rush to add children to every component that actually makes use of it 😂

@OliverJAsh I guess it's because we can type-check much strictly by being able to distinguish components that have children and those do not. I was also always wondering why we could pass children down to those that don't specify children in their props. Now I know it's for backwards compatibility.

@raon0211 is correct. It's not sound to auto-inject children because some components simply do not take children. Not only that, but the auto-injected children is a whole mess of unioned types that basically allows anything, but some components have render props children, some take objects, some take arrays, etc. so enforcing that the user actually properly type their children like any other prop is more sound.

I will close the pull request. Thanks again, I learned some today. Couldn’t think of the render props.

Why PR is closed?
Problem is not solved.
https://codesandbox.io/s/x7wno1q04q

This issue is resolved because you can add children: React.ReactNode to Comp1's prop type. The argument is that the bug is in fact that children is added automatically to React.FunctionComponent props, not that it is explicitly needed with React.memo components.

@jacobrask does that mean that at some point React will stop injecting children prop to functional components? As of React 16.8, there's pretty much only one type of FunctionComponent left, and to the best of my knowledge it always have the children prop, which is at the moment covered correctly within this typings. If that's a bug as you say, could you kindly reference it in the React repository?

The issue arises when connecting a functional component with a Redux store using TS. Right now the latest version of @types/react-redux breaks all the React.FC that naively use children prop, which I reckon is a pretty common use case and can cause a lot of pain to many developers (as it did to me) . The reproduction is like the following:

const component:React.FC<{whatever?:string}> = 
    ({whatever, children})=>(<div>{children}</div>);
const ConnectedComponent=connect(mapStateToProps)(T);
[...]
return <ConnectedComponent>Any content</ConnectedComponent>; // this causes a typescript compiler error

ConnectedComponent in this example has some subtype of ExoticComponent, and I have no idea whether that's an expected behaviour, but that issue with @types/react-redux led me here from the aforementioned thread with @OliverJAsh, and I think more discussion / explanation on the subject is required from the maintainer team of @types/react. If that's an expected behaviour, seems like @types/react should not allow destructurizing children from props in the first place, but even if it would, that doesn't change the actual behaviour of the system, does it?