Export the DevSettings module, add addMenuItem method

[janicduplessis]

Summary

I wanted to configure the RN dev menu without having to write native code. This is pretty useful in a greenfield app since it avoids having to write a custom native module for both platforms (and might enable the feature for expo too).

This ended up a bit more involved than planned since callbacks can only be called once. I needed to convert the DevSettings module to a NativeEventEmitter and use events when buttons are clicked. This means creating a JS wrapper for it. Currently it does not export all methods, they can be added in follow ups as needed.

Changelog

[General] [Added] - Export the DevSettings module, add addMenuItem method

Test Plan

Tested in an app using the following code.

if (__DEV__) {
 DevSettings.addMenuItem('Show Dev Screen', () => {
    dispatchNavigationAction(
      NavigationActions.navigate({
        routeName: 'dev',
      }),
    );
  });
}

Added an example in RN tester

[janicduplessis]

Is this okay? This header is from the DevSupport target. Otherwise I could check if bridge.devMenu exists at runtime.

[cpojer]

I like the idea of this but I'm not sure if this is a good idea. The dev menu should work when JS is not available. Adding options from JS means they won't be shown when reloading and Metro goes offline for example.

cc @rickhanlonii

[janicduplessis]

@cpojer This doesn’t really change the way the dev menu works, it simply allows to add addtional options from JS once the bundle is loaded. This is mostly useful in the context of a greenfield app.

The added menu options also still work when metro is offline as long as a js bundle is present.

[janicduplessis]

An example use case here is a Dev only screen in the context of a greenfield app (js screen + navigation) the option to open this screen should only be available when js is loaded anyway. Adding menu options from native is still supported for when it is needed, if the option need to be available before the js bundle is loaded.

[janicduplessis]

ping @cpojer :)

[cpojer]

Can you add the right flow types for NativeDevSettings?

[cpojer]

Ehh, meant to request changes…

[cpojer]

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?

[janicduplessis]

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.

[cpojer]

Thanks, I hate it!

Do you mind rebasing and making the flow type change?

[janicduplessis]

@cpojer Done!

[cpojer]

Awesome!

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[cpojer]

Your PR doesn't compile on Android and iOS. The Android error you can see on CI, the iOS one from fb internal:

React/Modules/RCTDevSettings.mm:338:10: error: unknown type name 'typeof'; did you mean 'typedef'?
  __weak typeof(self) weakSelf = self;

[janicduplessis]

@cpojer Oups, weird that the android one doesn't happen with gradle. Should be good now.

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @janicduplessis in cc068b0.

^{When will my fix make it into a release? | Upcoming Releases}

[rickhanlonii]

@janicduplessis did this make it into the docs?

[janicduplessis]

@rickhanlonii I don’t remember doing it so probably not 😬

[rickhanlonii]

No sweat! Issue filed here to add it facebook/react-native-website#1523

[mohamedsgap]

Hey guys! @shergin @rickhanlonii @cpojer @janicduplessis
just finished writing doc for DevSettings module 'addMenuItem' as an API reference and need you to review PR to see if it seems good or need some changes!

[slorber]

@janicduplessis , thanks for this useful addMenuItem

Wonder if this does not make sense to provide a way to "remove" this menu item too?

This way, we could add contextual menu items.

useEffect(() => {
  if (__DEV__) {
    const remove = DevSettings.addMenuItem('Fill current form', () => {
       fillCurrentForm()
    });
    return remove;
  }
},[])

[janicduplessis]

@slorber Yea I think it makes sense, I kept the initial PR as simple as possible but it should be possible to implement this.