Fix issue #6300: Improve error message for unregistered callbacks. #6436
Conversation
Fix for issue facebook#6300. This fix adds a more descriptive error message for the error case when more than one callback is registered to a native function.
|
By analyzing the blame information on this pull request, we identified @tadeuzagallo, @davidaurelio and @nicklockwood to be potential reviewers. |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
| @@ -192,9 +192,14 @@ class MessageQueue { | |||
| let debug = this._debugInfo[cbID >> 1]; | |||
| let module = debug && this._remoteModuleTable[debug[0]]; | |||
| let method = debug && this._remoteMethodTable[debug[0]][debug[1]]; | |||
| let errorMessage = `Callback with id ${cbID}: ${module}.${method}() not found`; | |||
| if (method != null || method != undefined) { | |||
There was a problem hiding this comment.
eqeqeq: Expected '!==' and instead saw '!='.
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
@agiron123 updated the pull request. |
| invariant( | ||
| callback, | ||
| `Callback with id ${cbID}: ${module}.${method}() not found` | ||
| errorMessage | ||
| ); |
There was a problem hiding this comment.
Can you move this whole thing (L192-L203) inside an if (!callback)? We can save all the array accesses + string concatenation in the common case.
There was a problem hiding this comment.
Added to code. I was able to surround everything with the if(!callback) statement except for the module and method variable declaration lines, as they are used both in the if statement and after the if statement. I really like this change. Nice and clean.
|
@agiron123 updated the pull request. |
…ron123/react-native into unregisteredCallbackErrorImprovement
|
@agiron123 updated the pull request. |
|
@tadeuzagallo Any more feedback? |
|
@agiron123 No, that looks good, sorry for the delay. @facebook-github-bot shipit |
facebook-github-bot
added
GH Review: accepted
and removed
GH Review: review-needed
labels
|
Thanks for importing. If you are an FB employee go to Phabricator to review. |
ghost
closed this
in
…acks. Summary:Fix for issue facebook#6300: Motivation: When more than one callback is registered to a native module, the error message that a user receives is not indicative of what is really happening. Closes facebook#6436 Differential Revision: D3087551 Pulled By: tadeuzagallo fb-gh-sync-id: 93c703348dc53b75c5b507edc71754680ab5c438 shipit-source-id: 93c703348dc53b75c5b507edc71754680ab5c438
Fix for issue #6300:
Motivation: When more than one callback is registered to a native module, the error message that a user receives is not indicative of what is really happening.
Test Plan:
MessageQueue. __invokeCallback(cbID, args) will need tests added.
Will need to add appropriate mocks for this._remoteMethodTable to return null and undefined.
Will need to test the cases when debug && this._remoteMethodTable[debug[0]][debug[1]];
returns null, undefined, and a non-null value.
Finally, will need to ensure that invariant(callback, errorMessage) function is called with the appropriate parameters.
As of right now, the code does not have unit tests attached. I can add them later if needed.
Demonstrate the code is solid:
To fix this issue, I first reproduced the conditions of the following issue:
#6286
To do so, I used the Movies sample project.
After successfully reproducing the error, I then followed the stack trace to come to the appropriate fix.
I can post the code to reproduce the issue if needed. It is essentially the same as that of issue #6286.
Let me know if you have any questions or concerns.
Commit Message:
Fix for issue #6300.
This fix adds a more descriptive error message for the error case when
more than one callback is registered to a native function.