Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
pomle opened this issue Oct 20, 2020 · 8 comments
Closed

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

pomle opened this issue Oct 20, 2020 · 8 comments
Labels
BASIC good first issue Hacktoberfest help wanted wontfix

Comments

@pomle
Copy link

@pomle pomle commented Oct 20, 2020

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;
  1. 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."

Screenshot 2020-10-20 at 19 17 56

The expected behavior

The error should have been detected at compile time.

@sw-yx
Copy link
Collaborator

@sw-yx sw-yx commented Oct 20, 2020

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

@sw-yx sw-yx added BASIC good first issue Hacktoberfest help wanted labels Oct 20, 2020
@pomle
Copy link
Author

@pomle pomle commented Oct 21, 2020

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

@sw-yx
Copy link
Collaborator

@sw-yx sw-yx commented Oct 21, 2020

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
Copy link
Author

@pomle pomle commented Oct 21, 2020

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.

@sw-yx
Copy link
Collaborator

@sw-yx sw-yx commented Oct 21, 2020

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

@pomle
Copy link
Author

@pomle pomle commented Oct 21, 2020

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

@stale
Copy link

@stale stale bot commented Dec 20, 2020

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!

@stale stale bot added the wontfix label Dec 20, 2020
@sw-yx
Copy link
Collaborator

@sw-yx sw-yx commented Dec 20, 2020

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

chef1119 added a commit to chef1119/chef-typescript that referenced this issue Oct 20, 2021
bernssolg added a commit to bernssolg/My-React-Sample that referenced this issue Feb 28, 2022
erinodev added a commit to erinodev/My-React-project that referenced this issue Feb 28, 2022
DarlingUUi added a commit to DarlingUUi/React that referenced this issue Apr 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BASIC good first issue Hacktoberfest help wanted wontfix
Projects
None yet
Development

No branches or pull requests

2 participants