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

The wrapped component itself might not accept children though, in which case the exotic component has unsoundly added an extra available attribute. The actual fix in the example code would be to properly type props in the argument to forwardRef, or use the second generic type parameter to properly type the props:

const WrappedPlayer = React.forwardRef<HTMLAudioElement, JSX.IntrinsicElements['audio']>((props, ref) => ( <audio autoPlay ref={ref} {...props} /> ));
// or 
const WrappedPlayer = React.forwardRef<HTMLAudioElement>((props: JSX.IntrinsicElements['audio'], ref) => ( <audio autoPlay ref={ref} {...props} /> ));

@ferdaber

Yeah it was a change I never really agreed with... but I can understand why it would be convenient.

It makes it difficult to narrow the type of children if your component only accepts render functions, for example, and this predated render functions being super popular; but in the spirit of consistency I suppose it makes sense.

I'm going to abstain from reviewing so another maintainer can take a look so you can get their thoughts on it too.

@pkeuter One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

This problem does give me an idea for a convenience overload for forwardRef itself:

type RefTypeOfComponent<T extends ReactType> = T extends any
    ? Required<ComponentPropsWithRef<T>> extends { ref: Ref<infer R> }
        ? R
        : never
    : never;

function forwardRef<T extends ReactType>(
    Component: RefForwardingComponent<RefTypeOfComponent<T>, ComponentPropsWithoutRef<T>>
): ForwardRefExoticComponent<ComponentPropsWithRef<T>>;

@Jessidhia, @ferdaber, thanks for both of your comments. I can agree with the fact that adding { children?: React.ReactNode } might not be the best way to go here.

I don't really understand though, that if "magic children" are an issue, that the behaviour is so different between types. Shouldn't this be more coherent?

The method you're proposing seems nice! Are you going to add it? It doesn't really solve the other issue though. Is that already being addressed?

If we were able to rewrite React types, children would not be built-in to the SFC type and the React.Component base props class property, because it leads to the unsoundness that @Jessidhia mentioned.

With the popularity of class based components it became an expectation; but actually un-annotated function components would not by default be allowed to accept children. Maybe we can move away from this convenience and have users actually strongly type children if their components accept them, by not adding to the cruft to exotic component types.

That would IMHO at least be consistent and less confusing than it is now. But it would also mean that users would have to change their existing convenient methods (or just not upgrade).

Yeah, I'm thinking that when we get better JSX facilities from the TS team, and when React 17 comes out, we can do a much bigger breaking change for all this stuff. I agree though that at this point it's not clear when children as a prop is auto added.

I think right now the most consistent convention is to never expect children to be auto added, and to explicitly add it to your props interface when you expect them. They will always work with the auto added children anyways.

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

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