[ButtonBase] Extend error message for invalid component prop
#15627
Conversation
eps1lon
added
docs
|
@material-ui/core: parsed: +0.07% , gzip: +0.17% Details of bundle changes.Comparing: 1555e52...78cfcfc
|
| @@ -89,7 +89,18 @@ class ButtonBase extends React.Component { | |||
| }); | |||
|
|
|||
| componentDidMount() { | |||
| prepareFocusVisible(this.getButtonNode().ownerDocument); | |||
| const button = this.getButtonNode(); | |||
| if (button == null) { | |||
There was a problem hiding this comment.
Do we know if the Jest issue is actually solvable or a limitation that can't be solved?
There was a problem hiding this comment.
I would suspect it's not actionable for the users: #15598 (comment).
There was a problem hiding this comment.
Ok, I could create a simple reproduction.
There was a problem hiding this comment.
This is the beginning of an answer: facebook/react#7740. I see that it's a common problem in the ecosystem: Popmotion/popmotion#736. I'm not sure that we can move forward in this direction.
There was a problem hiding this comment.
MenuList has the same issue. Probably others as well.
@eps1lon I'm not advocating that we try to have the whole library support react-test-renderer. Menu would have already had this issue in v3, so we aren't likely impacting anyone new there. I am mainly just concerned about ButtonBase being a new place where this is an issue. Just about every page in most apps will have buttons and other things that leverage ButtonBase. I would expect that it is the most-used component.
There was a problem hiding this comment.
@ryancogswell I think that it's fine to give core contributors a free pass when they feel strongly about something. The recovery cost is cheap with this change. If we really need to change the approach in the future, we can. The advantage of being strict is that we will learn more about our developers use cases. They can always open a new issue about the problem, as well as upvote it.
There was a problem hiding this comment.
react-test-renderer runs layout effects though which means e.g. a MenuList would crash because focus isn't mocked. I created a small repo to illustrate the issues and I can only come to the conclusion that react-test-renderer is not viable for our components. I couldn't get it to work for Modal.
There was a problem hiding this comment.
Aside: tests already crash in codesandbox when importing our components. We should also investigate that.
There was a problem hiding this comment.
I'm fine with that. I just think this would have been something that would have been nice for users to have longer warning about from a migration standpoint. I agree that it will be useful to know how many people really have a problem with this and how painful it will or won't be for them to address.
eps1lon
force-pushed
the
feat/ButtonBase/improve-component-warning
branch
from
112fe23 to
57cff46
Compare
Co-Authored-By: eps1lon <silbermann.sebastian@gmail.com>
eps1lon
force-pushed
the
feat/ButtonBase/improve-component-warning
branch
from
57cff46 to
92d984b
Compare
|
The GitHub out of order message problem makes it hard to follow the conversation. I'm proposing the following plan of action:
|
I found it very interesting earlier today to see the two of you making comments in the future (e.g. oliviertassinari commented 4 hours from now). Apparently, the issue was related to bad handling of time zones. |
Closes #15598
Throw a custom error instead of a cryptic
Cannot read ownerDocument of nullwhich just points to the console warnings that explain the issue in more details with links to fix prescriptions.Adds a redirect from https://next.material-ui.com/bug to the bug report issue template.
My preferred solution would be a custom error boundary around all our components that catches and rethrows errors while also prefixing error messages with a hint to the console and helpful links. This would be a dev only boundary which requires some thought about build infra etc. But it's definitely something I want to explore later. For now this custom solution should be sufficient.