Skip to content
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

Update native-modules-ios.md #2768

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Huang-Libo
Copy link
Contributor

@netlify
Copy link

netlify bot commented Sep 5, 2021

✔️ Deploy Preview for react-native ready!

🔨 Explore the source changes: 35fd9f1

🔍 Inspect the deploy log: https://app.netlify.com/sites/react-native/deploys/61386ee8bb398f0007f540e9

😎 Browse the preview: https://deploy-preview-2768--react-native.netlify.app

#import <React/RCTEventEmitter.h>

@interface CalendarModule : RCTEventEmitter <RCTBridgeModule>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason behind removing RCTBridgeModule?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: The Native Modules docs have been mostly rewritten not that long time ago in #2261, and we have fairly tested the example codes then, so I'm a bit surprised to see this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Create Custom Native Module Files, it create RCTCalendarModule.h:

//  RCTCalendarModule.h
#import <React/RCTBridgeModule.h>
@interface RCTCalendarModule : NSObject <RCTBridgeModule>
@end

Then, in Sending Events to JavaScript, it says:

Update your header class to import RCTEventEmitter and subclass RCTEventEmitter:

The code is :

//  CalendarModule.h

#import <React/RCTBridgeModule.h>
#import <React/RCTEventEmitter.h>

@interface CalendarModule : RCTEventEmitter <RCTBridgeModule>
@end

I think the name should be RCTCalendarModule as before. So the code should be:

//  RCTCalendarModule.h

#import <React/RCTBridgeModule.h>
#import <React/RCTEventEmitter.h>

@interface RCTCalendarModule : RCTEventEmitter <RCTBridgeModule>
@end

That is, implement RCTEventEmitter in the native module.

But then, The class in the sample code is changed to CalendarManager.m, it override the methode in RCTEventEmitter :

@implementation CalendarManager
{
  bool hasListeners;
}

// Will be called when this module's first listener is added.
-(void)startObserving {
    hasListeners = YES;
    // Set up any upstream listeners or background tasks as necessary
}

// Will be called when this module's last listener is removed, or on dealloc.
-(void)stopObserving {
    hasListeners = NO;
    // Remove upstream listeners, stop unnecessary background tasks
}

- (void)calendarEventReminderReceived:(NSNotification *)notification
{
  NSString *eventName = notification.userInfo[@"name"];
  if (hasListeners) { // Only send events if anyone is listening
    [self sendEventWithName:@"EventReminder" body:@{@"name": eventName}];
  }
}

That is, implement RCTEventEmitter in independent CalendarManager.

The problem is that CalendarModule.h and CalendarManager.m do not match.

We can inherit RCTEventEmitter from either CalendarModule or CalendarManager, and using independent CalendarManager is more decoupled.

So I DELETE CalendarModule.h, ADD CalendarManager.h:

//  CalendarManager.h

#import <React/RCTEventEmitter.h>

@interface CalendarManager : RCTEventEmitter
@end

The RCTBridgeModule protocol is not required for simple use, so I did not add it. For example, RCTKeyboardObserver and RCTStatusBarManager inherited from RCTEventEmitter in react-core do not comply with the RCTBridgeModule protocol.

Copy link
Collaborator

@Simek Simek Sep 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the deep dive into the logic behind the changes, I really enjoy reading that part. 🙂

The RCTBridgeModule protocol is not required for simple use, so I did not add it. For example, RCTKeyboardObserver and RCTStatusBarManager inherited from RCTEventEmitter in react-core do not comply with the RCTBridgeModule protocol.

The problem is that this guide should cover most common and standard use case and bridge removal might seems link nice optimization, but lack of it will break most of the Native Modules anyway and probably some of the website users following the tutorial will report issues with missing bridge in their cases.

So it would be nice to retain the Bridge here, other than that the changes looks good, great work! 👍

Also please fix the lint warnings, without that I cannot merge this in.

docs/native-modules-ios.md Outdated Show resolved Hide resolved
docs/native-modules-ios.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Huang-Libo Huang-Libo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete comment

#import <React/RCTEventEmitter.h>

@interface CalendarModule : RCTEventEmitter <RCTBridgeModule>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Create Custom Native Module Files, it create RCTCalendarModule.h:

//  RCTCalendarModule.h
#import <React/RCTBridgeModule.h>
@interface RCTCalendarModule : NSObject <RCTBridgeModule>
@end

Then, in Sending Events to JavaScript, it says:

Update your header class to import RCTEventEmitter and subclass RCTEventEmitter:

The code is :

//  CalendarModule.h

#import <React/RCTBridgeModule.h>
#import <React/RCTEventEmitter.h>

@interface CalendarModule : RCTEventEmitter <RCTBridgeModule>
@end

I think the name should be RCTCalendarModule as before. So the code should be:

//  RCTCalendarModule.h

#import <React/RCTBridgeModule.h>
#import <React/RCTEventEmitter.h>

@interface RCTCalendarModule : RCTEventEmitter <RCTBridgeModule>
@end

That is, implement RCTEventEmitter in the native module.

But then, The class in the sample code is changed to CalendarManager.m, it override the methode in RCTEventEmitter :

@implementation CalendarManager
{
  bool hasListeners;
}

// Will be called when this module's first listener is added.
-(void)startObserving {
    hasListeners = YES;
    // Set up any upstream listeners or background tasks as necessary
}

// Will be called when this module's last listener is removed, or on dealloc.
-(void)stopObserving {
    hasListeners = NO;
    // Remove upstream listeners, stop unnecessary background tasks
}

- (void)calendarEventReminderReceived:(NSNotification *)notification
{
  NSString *eventName = notification.userInfo[@"name"];
  if (hasListeners) { // Only send events if anyone is listening
    [self sendEventWithName:@"EventReminder" body:@{@"name": eventName}];
  }
}

That is, implement RCTEventEmitter in independent CalendarManager.

The problem is that CalendarModule.h and CalendarManager.m do not match.

We can inherit RCTEventEmitter from either CalendarModule or CalendarManager, and using independent CalendarManager is more decoupled.

So I DELETE CalendarModule.h, ADD CalendarManager.h:

//  CalendarManager.h

#import <React/RCTEventEmitter.h>

@interface CalendarManager : RCTEventEmitter
@end

The RCTBridgeModule protocol is not required for simple use, so I did not add it. For example, RCTKeyboardObserver and RCTStatusBarManager inherited from RCTEventEmitter in react-core do not comply with the RCTBridgeModule protocol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants