[Fiber] Use T | null instead of ?T types
#8978
Conversation
Flow maybe types accept both null and undefined. When the final parameter of a function accepts a maybe type, passing nothing (undefined) successfully typechecks, which can lead to bugs if the omission is accidental. Being forced to pass null is harder to screw up. Explicit null checks are also faster.
sophiebits
left a comment
sophiebits
left a comment
There was a problem hiding this comment.
Didn't verify but I trust you. Did you only change checks against fibers or also others? Tests all pass?
| // that was created by the user-provided function. | ||
| if (!isPerformingWork && !isBatchingUpdates) { | ||
| performWork(TaskPriority); | ||
| performWork(TaskPriority, null); |
There was a problem hiding this comment.
Nice that we caught that.
There was a problem hiding this comment.
I actually don't know why Flow didn't catch this before, because the type on performWork was already Deadline | null
There was a problem hiding this comment.
@acdlite I found flow would catch this if Deadline was declared in ReactFiberScheduler.More details facebook/flow#3368
|
@spicyj Mainly the Fiber type but I fixed most of the other optional types I found, too, with exceptions for things like user-facing functions. Possible there are a few that I missed. |
|
This broke some stuff. At least |
|
Ah. Actually I think the validation we do is correct, but we have an internal component that doesn't validate the ref. Interesting. |
6 participants
Flow maybe types accept both null and undefined. When the final parameter of a function accepts a maybe type, passing nothing (undefined) successfully typechecks, which can lead to bugs if the omission is accidental. Being forced to pass null is harder to screw up.
Explicit null checks are also faster.