React.ReactNode allows passing {} as children which will crash at runtime

[pomle]

In https://github.com/typescript-cheatsheets/react#useful-react-prop-type-examples it says

export declare interface AppProps {
  children: React.ReactNode; // best, accepts everything
}

ReactNode produces a false positive type check for {}.

React version: 16.14

Steps To Reproduce

1. Create a new CRA project with TypeScript using for example yarn create react-app repro --template typescript
2. Accept a prop using React.ReactNode as type in default App component.

import React from 'react';

type AppProps = {
  children: React.ReactNode;
}

function App({children}: AppProps) {
  return <div>{children}</div>;
}

export default App;

3. Update index.tsx and supply {} as children to ```

import React from 'react';
import ReactDOM from 'react-dom';
import './index.css';
import App from './App';
import * as serviceWorker from './serviceWorker';

ReactDOM.render(
  <React.StrictMode>
    <App>{{}}</App>
  </React.StrictMode>,
  document.getElementById('root')
);

// If you want your app to work offline and load faster, you can change
// unregister() to register() below. Note this comes with some pitfalls.
// Learn more about service workers: https://bit.ly/CRA-PWA
serviceWorker.unregister();

The current behavior

Application crash at runtime with the error "Objects are not valid as a React child (found: object with keys {}). If you meant to render a collection of children, use an array instead."

The expected behavior

The error should have been detected at compile time.

[swyxio]

damn fantastic point. do you have an idea for alternative recommendation for typing children?

[pomle]

@sw-yx, I am trying to figure out where this problem comes from still.

Correct me if I am wrong; this repo assumes types are derived from DefinitelyTyped?

Then, either the recommendation is wrong.

But it is possible that the type ReactNode from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L239 is wrong.

It inherits {} from type ReactFragment. I would not bet my life on it yet, but I think {} is way to wide for a ReactFragment.

Looking at the React source (typed in Flow afaict) it does not seem to agree
https://github.com/facebook/react/blob/master/packages/shared/ReactTypes.js#L20

Do you have any more insight, perhaps?

[swyxio]

well FYI the recommendation comes from me when I was learning R+TS so it's def not official. you're the first person in 2 years that has come along and raised any issue about it!

and as for the typing and whether they could be better, this is a @ferdaber and @eps1lon issue. I bet this has been raised before and we might learn something here about why React.Fragment has to accept the invalid {}.

[pomle]

Gotcha! Found the open issue at DefinitelyTyped/DefinitelyTyped#37596

Thanks for giving more context. I will keep this open until I understand more about the problem @DefinitelyTyped.

The next steps from me will be:

• See if we can do something non-breaking to improve typing React nodes.
• See if we can do a breaking change.
• Recommend an alternate type from this repo and document the pitfalls.

[swyxio]

very reasonable. we can also leave a small warning to others in our notes, as a simple first step.

[pomle]

@sw-yx, absolutely. You know best what the readers will benefit from.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions!

[swyxio]

added the warning as it is unlikely to be resolved for now