React.ReactNode allows passing {} as children which will crash at runtime
pomle opened this issue · 8 comments
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
- Create a new CRA project with TypeScript using for example
yarn create react-app repro --template typescript
- 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;
- 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.
damn fantastic point. do you have an idea for alternative recommendation for typing children?
@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?
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 {}.
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.
very reasonable. we can also leave a small warning to others in our notes, as a simple first step.
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!
added the warning as it is unlikely to be resolved for now