Dedupe warnings about refs on functional components #8739
Conversation
It's not a big problem for string refs because the ref stays the same and the warning code path runs once per mount. However, it is super annoying for functional refs since they're often written as arrows, and thus the warning fires for every render. Both tests are currently failing since we're mounting twice, so even the string ref case prints warnings twice.
We don't want to run getStackAddendumByID() unless we actually know we're going to print the warning. This doesn't affect correctness. Just a performance fix.
This fixes the duplicate warnings and adds an additional test for corner cases. Our goal is to print one warning per one offending call site, when possible. We try to use the element source for deduplication first because it gives us the exact JSX call site location. If the element source is not available, we try to use the owner name for deduplication. If even owner name is unavailable, we try to use the functional component unique identifier for deduplication so that at least the warning is seen once per mounted component.
|
@iamdustan I can't assign you in GH but I'd appreciate your review too. |
| ReactDebugCurrentFiber.getCurrentFiberStackAddendum() | ||
| ); | ||
| let warningKey = ownerName || workInProgress._debugID || ''; | ||
| const debugSource = workInProgress._debugSource; |
There was a problem hiding this comment.
_debugSource is added via a babel plugin, right?
There was a problem hiding this comment.
It is added here to the Fiber, and it is on the element from this Babel plugin (enabled by default in CRA).
There was a problem hiding this comment.
not directly related to this PR, but are there any concerns that dev behavior may change slightly based on babel-plugins? Especially for compile-to-js implementations?
Perhaps there should be documentation added to https://facebook.github.io/react/contributing/implementation-notes.html for _debugSource?
There was a problem hiding this comment.
Would be nice to add, want to send a PR?
| spyOn(console, 'error'); | ||
|
|
||
| // Prevent the Babel transform adding a displayName. | ||
| var createClassWithoutDisplayName = React.createClass; |
There was a problem hiding this comment.
if the babel plugin is updated to follow React.createClass aliases I assume this test would break?
There was a problem hiding this comment.
Yea. But I bet pretty hard it won't happen. :-)
| expectDev(console.error.calls.count()).toBe(1); | ||
| }); | ||
|
|
||
| it('deduplicates ref warnings based on element or owner', () => { |
There was a problem hiding this comment.
+1. this test looks very comprehensive.
|
Thanks for taking a look! |
The warning added in #8635 is very noisy.
It was already somewhat noisy for strings, printing on every mount, but it is much more noisy for functional refs because they often change between updates, causing the warning to be printed again and again.
Before this PR, we would get warnings once per attempt to attach the ref.
After this PR, we get warnings once per callsite—to the best of our ability to tell them apart.
See individual commit messages for more information.