Fix suspenseCallback type warning, add a test #16194
Conversation
| @@ -1329,7 +1329,7 @@ function commitSuspenseComponent(finishedWork: Fiber) { | |||
| if (thenables !== null) { | |||
| suspenseCallback(new Set(thenables)); | |||
| } | |||
| } else if (__DEV__) { | |||
| } else if (__DEV__ && suspenseCallback != null) { | |||
There was a problem hiding this comment.
We try to use explicit comparisons instead of !=. Actually do we even want to support null? Maybe let's check for just undefined.
We also try to separate __DEV__ checks from others. Maybe:
else if (__DEV__) {
if (suspenseCallback !== undefined) {
warning(...)
}
}There was a problem hiding this comment.
Is the __DEV__ on a separate line still something we want? I was pulled up on it a few months ago and asked to move it inline (forgot where and who), but GCC appropriately handles it.
There was a problem hiding this comment.
This was more a legacy pattern when we didn't trust the compiler. We trust GCC now though so we don't have to put it on a separate line anymore.
However, I often still separate it out for clarity. I found it hard to see what the scope of deleted things are when they're on the same line. Especially if it's not clear what the precedence is like else if (__DEV__ && something || bar).
I've also seen cases where the order of execution is wrong like:
if (foo() && __DEV__) { // wrong order
}
So separating it out helps understanding whether the foo() call will remain or not.
|
No significant bundle size changes to report. Generated by 🚫 dangerJS |
| @@ -1329,7 +1329,7 @@ function commitSuspenseComponent(finishedWork: Fiber) { | |||
| if (thenables !== null) { | |||
| suspenseCallback(new Set(thenables)); | |||
| } | |||
| } else if (__DEV__) { | |||
| } else if (__DEV__ && suspenseCallback != null) { | |||
There was a problem hiding this comment.
We usually check undefined and null separately (partly to avoid document.all being considered).
Sorry for the churn. Added a test.