-
Notifications
You must be signed in to change notification settings - Fork 50k
Add callback validation to fiber-based renderers #8677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add callback validation to fiber-based renderers #8677
Conversation
|
Nit: can we also change Stack to use it? |
|
Yeah, I was already making that change. 😁 Realized they were super redundant after I pushed the Fiber change. |
|
It's only ever used in |
I don't understand your comment @acdlite. |
|
To clarify: rather than validating separately in |
|
There's nothing renderer specific about |
5559da7 to
8d9a1ad
Compare
|
Thanks for clarifying @acdlite. That's a nice improvement since it will also add callback validation for other renderer types. This PR has been updated with that change. |
| // | ||
| // If the update is cloned, it returns the cloned update. | ||
| function insertUpdate(fiber : Fiber, update : Update, methodName : ?string) : Update | null { | ||
| function insertUpdate(fiber : Fiber, update : Update, methodName : string) : Update | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since methodName is now always a string, we can remove the typeof check on line 215.
acdlite
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
8d9a1ad to
bc05ef1
Compare
| formatUnexpectedArgument(callback) | ||
| ); | ||
| }, | ||
| validateCallback: validateCallback, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this indirection useful? Can we just import it directly in the calling file instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't! Good catch. Removed. 😄
Moved ReactFiberClassComponent validateCallback() helper function into a standalone util used by both fiber and stack implementations. Validation now happens in ReactFiberUpdateQueue so that non-DOM renderers will also benefit from it.
bc05ef1 to
8fbcd49
Compare
|
Another one we can easily make a warning. Should we do a pass through and see how many of these we can make into warnings to help file size? |
|
Nit: Same as #8639 (comment) |
Yes, I'll add to umbrella. |
Moved
ReactFiberClassComponentvalidateCallback()helper function into a standalone util used by both fiber and stack implementations. Validation now happens inReactFiberUpdateQueueso that non-DOM renderers will also benefit from it.This fixes 2 failing fiber tests.