Remove MissingNativeRCTNetworkingShim; revert MissingNativeAppStateShim

[statm]

Summary

setupDevtools.js is accessing AppState.currentState without checking its availability. In environments where 1) __DEV__ == true, and 2) no RCTAppState native module is provided thus resorting to MissingNativeAppStateShim, this will result in an exception:

Cannot use 'AppState' module when native 'RCTAppState' is not included in the build. Either include it, or check 'AppState'.isAvailable before calling any methods.

(Interestingly, MissingNativeAppStateShim.currentState did have a default null value that was later removed.)

Update: Following @cpojer's suggestion of reverting a93b7a2. Title also updated to reflect this.

Changelog

[General] [Fixed] - Remove MissingNativeRCTNetworkingShim; revert MissingNativeAppStateShim

Test Plan

No crashes in the same environments.

[facebook-github-bot]

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. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[cpojer]

Hey @statm,

thanks for this PR. I actually think this is probably not the right solution but you linked to the PR that introduced this change and I dug into why it was made in the first place. Long story short, I think we don't need all the complexity that was introduced in a93b7a2 at all. Would you mind simply making a revert of that commit and updating your PR with it? That should get rid of the isAvailable check (WebSocket should always be there in RN) and also restore the behavior from before where the currentState is simply null and doesn't throw. What do you think?

[cpojer]

Also please do sign the CLA :)

[analysis-bot]

Code analysis results:

• eslint found some issues. Run yarn lint --fix to automatically fix problems.

[analysis-bot]

Code analysis results:

• eslint found some issues. Run yarn lint --fix to automatically fix problems.

[cpojer]

@statm can you verify that this fixes the issue you are trying to solve and also can you sign the CLA under code.facebook.com/cla? Otherwise I can't merge this.

[statm]

@cpojer Sure! I'm currently working/waiting on 3 things:

• Install local react-native package in e2e #24381 so CI will pass
• Legal folks (MS) adding me to CLA
• Locally re-bundle and validate

[statm]

Hi @cpojer, reverting a93b7a2 did fix the crash, thanks for your suggestion.

We are running RN in a unit test environment, therefore have stripped out most default native modules, including AppState. Further investigation revealed that when AppState native module is not present, this code in setupDevtools.js will assume app is active, and attempt connections to reactDevTools via WebSocket:

const isAppActive = () => AppState.currentState !== 'background';

These connections will timeout after a few seconds, slowing down all tests that involve JS code. I'm considering adding a check here, like in my initial commit:

const isAppActive = () => AppState.isAvailable && AppState.currentState !== 'background';

When AppState native module is absent, do not connect to reactDevTools. What do you think?

Also - sorry about the CLA situation. It might take a little while :(

[cpojer]

@statm let me know once the CLA is figured out and then I can merge it :)

I'd prefer to handle the issue about unit test environment that you mentioned in a separate PR if possible (this revert needs to happen either way). First though I'd like to understand which test framework you are using and whether we could shim out this setup file altogether? In general I'm hesitant to change code to make unit testing easier when you can easily mock things like this with a framework like Jest.

[statm]

@cpojer Our UTs run on a native (C++) framework, focusing on managing facebook::react::Instances. You can think of them like a bunch of e2e tests, each with its own pre-built bundle. I'll try to add a dummy AppState module that always report 'background' just for testing and see if it stops devtools connection.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[statm]

@cpojer CLA done :)

[cpojer]

Awesome! Thanks so much for removing code :)

[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 @statm in 57c1a7a.

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