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

Improve error message when a map passed by mistake as a child to fragment #555

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

holyjak
Copy link
Collaborator

@holyjak holyjak commented Oct 27, 2024

... or to ErrorBoundary (which uses comp/fragment).

Without this change, we assume the map is props, and end up getting a warning like "Warning: Invalid prop some-prop... supplied to React.Fragment. React.Fragment can only have key and children props."

With this change, we get a proper error about trying to render the wrong type, and in which component.

WHY: The warning makes it harder to understand what the actual problem is.

…ment

... or to ErrorBoundary (which uses comp/fragment).

Without this change, we assume the map is props, and end up getting a _warning_ like "Warning: Invalid prop `some-prop...` supplied to `React.Fragment`. React.Fragment can only have `key` and `children` props."

With this change, we get a proper _error_ about trying to render the wrong type, and in which component.
@holyjak
Copy link
Collaborator Author

holyjak commented Oct 27, 2024

Testing: A simple app where Root has an arbitrary map as its body.

Of course a disadvantage of this change is that if fragment adds support for more properties in the future then we'd need to update the code. I am not sure whether we want to accept the risk or not.

Copy link
Member

@awkay awkay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React MAY add additional attributes in the future. I don't think this is what we should do. If you want to add something to the error-boundary for reporting an error I'd be more open to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants