Extend NativeBridge to support more events, use singleton for dispatching#1
Open
harunsmrkovic wants to merge 1 commit into
Open
Extend NativeBridge to support more events, use singleton for dispatching#1harunsmrkovic wants to merge 1 commit into
harunsmrkovic wants to merge 1 commit into
Conversation
…hing We were experiencing intermittent issues with the original code, sometimes receiving the error: Bridge is not set. This is probably because you've explicitly synthesized the bridge in NativeBridge, even though it's inherited from RCTEventEmitter. As proposed in this issue react/react-native#15421 (comment), solution is to use a singleton of NativeBridge, and then dispatch events via it.
JacobKingDev
suggested changes
Jul 31, 2019
JacobKingDev
left a comment
JacobKingDev
left a comment
There was a problem hiding this comment.
From the perspective of an Objective-C developer, the code looks mostly fine with only a couple of comments (mostly referring to the same issue).
|
|
||
| NSLog(@"digime: Got accounts %@", body); | ||
|
|
||
| [[NativeBridge allocWithZone: nil] emitEventWithName:@"accountsRetrieved" body: body]; |
There was a problem hiding this comment.
You've instantiated a singleton instance of NativeBridge above, but you're creating a new one each time here? Also, it's worth noting that the instance created here will be destroyed immediately after receiving the method invocation, which probably isn't a problem in this use case but could present an issue for long-lived processes.
Author
There was a problem hiding this comment.
I was honestly hoping that this code is doing the exact opposite of what you described 🙃
Can you write the proper code, and I can replace it?
| [[NativeBridge allocWithZone: nil] emitEventWithName:@"accountsRetrieved" body: body]; | ||
| } else { | ||
| NSLog(@"digime: Getting accounts failed %@", error); | ||
| [[NativeBridge allocWithZone: nil] emitEventWithName:@"accountsRetrieveFailed" body: error.localizedDescription]; |
| [self sendEventWithName:@"userAuthAccept" body:@""]; | ||
| [[DMEClient sharedClient] getFileList]; | ||
| NSLog(@"digime: Auth Success"); | ||
| [[NativeBridge allocWithZone: nil] emitEventWithName:@"userAuthAccept" body:@""]; |
| NSLog(@"Auth Deny"); | ||
| [self sendEventWithName:@"userAuthReject" body:@""]; | ||
| NSLog(@"digime: Auth Deny"); | ||
| [[NativeBridge allocWithZone: nil] emitEventWithName:@"userAuthDenied" body:@""]; |
| NSLog(@"Auth Failed"); | ||
| [self sendEventWithName:@"userAuthReject" body:@""]; | ||
| NSLog(@"digime: Auth Failed"); | ||
| [[NativeBridge allocWithZone: nil] emitEventWithName:@"userAuthFailed" body:@""]; |
| NSData * jsonData = [NSJSONSerialization dataWithJSONObject:files.fileIds options:0 error:&err]; | ||
| NSString * body = [[NSString alloc] initWithData:jsonData encoding:NSUTF8StringEncoding]; | ||
|
|
||
| [[NativeBridge allocWithZone: nil] emitEventWithName:@"fileListRetrieved" body: body]; |
| - (void)clientFailedToRetrieveFileList:(NSError *)error { | ||
| NSLog(@"Client failed to get file list"); | ||
| NSLog(@"digime: Client failed to get file list"); | ||
| [[NativeBridge allocWithZone: nil] emitEventWithName:@"fileListFailed" body:@""]; |
|
|
||
| [self sendEventWithName:@"fileData" body: stringData]; | ||
| NSLog(@"digime: Sending file data Event for %@", file.fileId); | ||
| [[NativeBridge allocWithZone: nil] emitEventWithName:@"fileRetrieved" body: stringData]; |
| - (void)fileRetrieveFailed:(NSString *)fileId error:(NSError *)error { | ||
| NSLog(@"file retrived failed"); | ||
| NSLog(@"digime: Failed retrieving file %@, %@", fileId, error); | ||
| [[NativeBridge allocWithZone: nil] emitEventWithName:@"fileRetrieveFailed" body: fileId]; |
| [[DMEClient sharedClient] getAccountsWithCompletion:^(CAAccounts * _Nullable accounts, NSError * _Nullable error) { | ||
|
|
||
| if (!error) { | ||
| NSError * err; |
There was a problem hiding this comment.
Please bump asterisk over to be next to name, IE NSError *err. We do have internal Objective-C semantics guidelines but I realise you can't see these!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We were experiencing intermittent issues with the original code, sometimes receiving the error:
Bridge is not set. This is probably because you've explicitly synthesized the bridge in NativeBridge, even though it's inherited from RCTEventEmitter.
As proposed in this issue react/react-native#15421 (comment), solution is to use a singleton of NativeBridge, and then dispatch events via it.