-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Export the DevSettings module, add addMenuItem
method
#25848
Closed
janicduplessis
wants to merge
2
commits into
facebook:master
from
janicduplessis:js-add-dev-menu-item
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
import NativeDevSettings from '../NativeModules/specs/NativeDevSettings'; | ||
import NativeEventEmitter from '../EventEmitter/NativeEventEmitter'; | ||
|
||
class DevSettings extends NativeEventEmitter { | ||
_menuItems: Map<string, () => mixed>; | ||
|
||
constructor() { | ||
super(NativeDevSettings); | ||
|
||
this._menuItems = new Map(); | ||
} | ||
|
||
addMenuItem(title: string, handler: () => mixed) { | ||
// Make sure items are not added multiple times. This can | ||
// happen when hot reloading the module that registers the | ||
// menu items. The title is used as the id which means we | ||
// don't support multiple items with the same name. | ||
const oldHandler = this._menuItems.get(title); | ||
if (oldHandler != null) { | ||
this.removeListener('didPressMenuItem', oldHandler); | ||
} else { | ||
NativeDevSettings.addMenuItem(title); | ||
} | ||
|
||
this._menuItems.set(title, handler); | ||
this.addListener('didPressMenuItem', (event) => { | ||
if (event.title === title) { | ||
handler(); | ||
} | ||
}); | ||
} | ||
|
||
reload() { | ||
NativeDevSettings.reload(); | ||
} | ||
|
||
// TODO: Add other dev setting methods exposed by the native module. | ||
} | ||
|
||
// Avoid including the full `NativeDevSettings` class in prod. | ||
class NoopDevSettings { | ||
addMenuItem(title: string, handler: () => mixed) {} | ||
reload() {} | ||
} | ||
|
||
module.exports = __DEV__ ? new DevSettings() : new NoopDevSettings(); |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
* @format | ||
* @flow | ||
*/ | ||
|
||
'use strict'; | ||
|
||
import * as React from 'react'; | ||
import {Alert, Button, DevSettings} from 'react-native'; | ||
|
||
exports.title = 'DevSettings'; | ||
exports.description = 'Customize the development settings'; | ||
exports.examples = [ | ||
{ | ||
title: 'Add dev menu item', | ||
render(): React.Element<any> { | ||
return ( | ||
<Button | ||
title="Add" | ||
onPress={() => { | ||
DevSettings.addMenuItem('Show Secret Dev Screen', () => { | ||
Alert.alert('Showing secret dev screen!'); | ||
}); | ||
}} | ||
/> | ||
); | ||
}, | ||
}, | ||
{ | ||
title: 'Reload the app', | ||
render(): React.Element<any> { | ||
return ( | ||
<Button | ||
title="Reload" | ||
onPress={() => { | ||
DevSettings.reload(); | ||
}} | ||
/> | ||
); | ||
}, | ||
}, | ||
]; |
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ | |
#import "RCTProfile.h" | ||
#import "RCTUtils.h" | ||
|
||
#import <React/RCTDevMenu.h> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this okay? This header is from the |
||
|
||
static NSString *const kRCTDevSettingProfilingEnabled = @"profilingEnabled"; | ||
static NSString *const kRCTDevSettingHotLoadingEnabled = @"hotLoadingEnabled"; | ||
static NSString *const kRCTDevSettingIsInspectorShown = @"showInspector"; | ||
|
@@ -111,8 +113,6 @@ @interface RCTDevSettings () <RCTBridgeModule, RCTInvalidating> { | |
|
||
@implementation RCTDevSettings | ||
|
||
@synthesize bridge = _bridge; | ||
|
||
RCT_EXPORT_MODULE() | ||
|
||
+ (BOOL)requiresMainQueueSetup | ||
|
@@ -152,8 +152,7 @@ - (instancetype)initWithDataSource:(id<RCTDevSettingsDataSource>)dataSource | |
|
||
- (void)setBridge:(RCTBridge *)bridge | ||
{ | ||
RCTAssert(_bridge == nil, @"RCTDevSettings module should not be reused"); | ||
_bridge = bridge; | ||
[super setBridge:bridge]; | ||
|
||
#if ENABLE_PACKAGER_CONNECTION | ||
RCTBridge *__weak weakBridge = bridge; | ||
|
@@ -197,6 +196,11 @@ - (void)invalidate | |
[[NSNotificationCenter defaultCenter] removeObserver:self]; | ||
} | ||
|
||
- (NSArray<NSString *> *)supportedEvents | ||
{ | ||
return @[@"didPressMenuItem"]; | ||
} | ||
|
||
- (void)_updateSettingWithValue:(id)value forKey:(NSString *)key | ||
{ | ||
[_dataSource updateSettingWithValue:value forKey:key]; | ||
|
@@ -210,7 +214,7 @@ - (id)settingForKey:(NSString *)key | |
- (BOOL)isNuclideDebuggingAvailable | ||
{ | ||
#if RCT_ENABLE_INSPECTOR | ||
return _bridge.isInspectable; | ||
return self.bridge.isInspectable; | ||
#else | ||
return false; | ||
#endif // RCT_ENABLE_INSPECTOR | ||
|
@@ -227,12 +231,12 @@ - (BOOL)isRemoteDebuggingAvailable | |
|
||
- (BOOL)isHotLoadingAvailable | ||
{ | ||
return _bridge.bundleURL && !_bridge.bundleURL.fileURL; // Only works when running from server | ||
return self.bridge.bundleURL && !self.bridge.bundleURL.fileURL; // Only works when running from server | ||
} | ||
|
||
RCT_EXPORT_METHOD(reload) | ||
{ | ||
[_bridge reload]; | ||
[self.bridge reload]; | ||
} | ||
|
||
RCT_EXPORT_METHOD(setIsShakeToShowDevMenuEnabled : (BOOL)enabled) | ||
|
@@ -285,10 +289,10 @@ - (void)_profilingSettingDidChange | |
BOOL enabled = self.isProfilingEnabled; | ||
if (self.isHotLoadingAvailable && enabled != RCTProfileIsProfiling()) { | ||
if (enabled) { | ||
[_bridge startProfiling]; | ||
[self.bridge startProfiling]; | ||
} else { | ||
[_bridge stopProfiling:^(NSData *logData) { | ||
RCTProfileSendResult(self->_bridge, @"systrace", logData); | ||
[self.bridge stopProfiling:^(NSData *logData) { | ||
RCTProfileSendResult(self.bridge, @"systrace", logData); | ||
}]; | ||
} | ||
} | ||
|
@@ -302,9 +306,9 @@ - (void)_profilingSettingDidChange | |
#pragma clang diagnostic push | ||
#pragma clang diagnostic ignored "-Wdeprecated-declarations" | ||
if (enabled) { | ||
[_bridge enqueueJSCall:@"HMRClient" method:@"enable" args:@[] completion:NULL]; | ||
[self.bridge enqueueJSCall:@"HMRClient" method:@"enable" args:@[] completion:NULL]; | ||
} else { | ||
[_bridge enqueueJSCall:@"HMRClient" method:@"disable" args:@[] completion:NULL]; | ||
[self.bridge enqueueJSCall:@"HMRClient" method:@"disable" args:@[] completion:NULL]; | ||
} | ||
#pragma clang diagnostic pop | ||
} | ||
|
@@ -329,6 +333,14 @@ - (BOOL)isHotLoadingEnabled | |
} | ||
} | ||
|
||
RCT_EXPORT_METHOD(addMenuItem:(NSString *)title) | ||
{ | ||
__weak __typeof(self) weakSelf = self; | ||
[self.bridge.devMenu addItem:[RCTDevMenuItem buttonItemWithTitle:title handler:^{ | ||
[weakSelf sendEventWithName:@"didPressMenuItem" body:@{@"title": title}]; | ||
}]]; | ||
} | ||
|
||
- (BOOL)isElementInspectorShown | ||
{ | ||
return [[self settingForKey:kRCTDevSettingIsInspectorShown] boolValue]; | ||
|
@@ -347,17 +359,17 @@ - (BOOL)isPerfMonitorShown | |
- (void)setExecutorClass:(Class)executorClass | ||
{ | ||
_executorClass = executorClass; | ||
if (_bridge.executorClass != executorClass) { | ||
if (self.bridge.executorClass != executorClass) { | ||
// TODO (6929129): we can remove this special case test once we have better | ||
// support for custom executors in the dev menu. But right now this is | ||
// needed to prevent overriding a custom executor with the default if a | ||
// custom executor has been set directly on the bridge | ||
if (executorClass == Nil && _bridge.executorClass != objc_lookUpClass("RCTWebSocketExecutor")) { | ||
if (executorClass == Nil && self.bridge.executorClass != objc_lookUpClass("RCTWebSocketExecutor")) { | ||
return; | ||
} | ||
|
||
_bridge.executorClass = executorClass; | ||
[_bridge reload]; | ||
self.bridge.executorClass = executorClass; | ||
[self.bridge reload]; | ||
} | ||
} | ||
|
||
|
@@ -386,7 +398,7 @@ - (void)_synchronizeAllSettings | |
|
||
- (void)jsLoaded:(NSNotification *)notification | ||
{ | ||
if (notification.userInfo[@"bridge"] != _bridge) { | ||
if (notification.userInfo[@"bridge"] != self.bridge) { | ||
return; | ||
} | ||
|
||
|
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
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.
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.
I don't feel good about using the string as identifier to connect them. Shall we use a numeric ID to keep track of them 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.
The reason for that is android already stores them by their name (https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerImpl.java#L309) so it would break later anyway if 2 items have the same name. It also makes it easier to not add the same item multiple times when hot reloading (see this comment https://github.com/facebook/react-native/pull/25848/files#diff-bd79be22516654291770eb407afd366fR14).
Not ideal but in practice for a dev only module I thought this was fine.
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.
Thanks, I hate it!
Do you mind rebasing and making the flow type change?