BugFix: Suspense priority warning firing when not supposed to #16256
Conversation
| @@ -1738,6 +1710,38 @@ describe('ReactSuspenseWithNoopRenderer', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| it('', async () => { | |||
| ); | ||
| expect(console.error.calls.argsFor(0)[1]).toContain( | ||
| 'A user-blocking update was suspended by:', | ||
| 'Warning: A user-blocking update was suspended by:', | ||
| ); | ||
| expect(console.error.calls.argsFor(0)[1]).toContain('AsyncText'); |
There was a problem hiding this comment.
I know this code already existed but curious — any reason we’re not using toWarnDev() for this warning?
There was a problem hiding this comment.
I think it was easier at the time to use console.error, because there was a lot of them. Do you prefer toWarnDev()?
|
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: db3ae32...c22465d react-art
react-dom
react-test-renderer
react-reconciler
react-native-renderer
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
This still adds all the suspended components to the component names lists. That seems misleading at best. Because if some other boundary during the same commit ended up suspending, then we'd show all the suspended components rather than just the new one.
It seems to me that we should only add the component names/promises that contributed to suspending.
I.e. we should move the logic from throwException into here: https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactFiberCompleteWork.js#L882-L896
|
@sebmarkbage Luna and I discussed this before she submitted the PR. The issue is that if we wait until the complete phase, we've lost track of the components that suspended in that subtree. So we'll need to track them via some mechanism, like on the stack. Or we could omit the names of the components that suspended from the message. I think the names of the components that triggered the update are the more relevant piece of information. Though arguably, the names of the components that suspended could also be helpful for debugging in the |
|
I was kind of hoping we could omit the names of the components that suspended, given that the implementation is annoying, and rely React DevTools to surface this information instead, eventually. |
|
I think what you really want is what suspended. As in what is the Promise associated with. That's why we're now attaching debug info to the promises. Not the nearest component that threw one. With call through and other optimizations we might not know the exact component. We should only need try/catch at boundary levels, not nearest component. This is unfortunate leak. So I'd be in favor removing that from the message. |
|
Ok @lunaruan in that case you can leave the Should also add a test where the component that triggers the update is in between the Suspense boundary and the component that throws. |
Previously, the suspense priority warning was fired even if the Root wasn't suspended. Changed the warning to fire only when the root is suspended.
Also refactored the suspense priority warning so it's easier to read.