Fixes iOS reload through metro "r" command key

[reyalpsirc]

Summary

This allows the iOS device to be reloaded through the metro command line, besides the fact that whenever packagerServerHost is called, it will only get the IP address once when debugging.

Changelog

[iOS] [Fixed] - Fixed connection of metro reload command to iOS device

Test Plan

• Build any react-native project in debug mode to an iOS device connected through USB
• Press the “r” key on the terminal that is running metro
• The device should now reload the project

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	6,605,133	-652
android	hermes	armeabi-v7a	6,281,181	-1,468
android	hermes	x86	6,945,142	-1,380
android	hermes	x86_64	6,875,012	-1,756
android	jsc	arm64-v8a	8,913,470	-588
android	jsc	armeabi-v7a	8,552,861	-1,411
android	jsc	x86	8,737,532	-1,305
android	jsc	x86_64	9,313,618	-1,696

Base commit: 6f627f6

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	966,896	-9,869,552

Base commit: 6f627f6

[elicwhite]

Can you provide some more context as to why this is the correct change? Also, please include a test plan that shows to the reviewers of this PR that this change works.

[reyalpsirc]

@TheSavior I think that this is the correct change because if no jsLocation is provided, the RCTBundleURLProvider will try to look for the device IP and use that to connect to metro.

The problem is that since that is only used for the RCTBridge, the RCTPackagerConnection does not have access to it and tries then to connect either through jsLocation or localhost.

Since iOS is not able to map localhost to the computer’s IP to which it is connected, it fails to connect to the websocket that allows the reload via “r” key on metro.

Based on that and knowing that the IP is the method used when no jsLocation is specified, in my opinion it makes sense to only get the IP once and then set it as the jsLocation since it will be the way to anything (from that time on) to connect to the remote computer.

[elicwhite]

Thanks for the context. I don't have any context on this code so I'll leave this PR for someone else to review / merge, but this additional info should make it easier for them.

[janicduplessis]

I don't like assigning jsLocation inside a getter method like this. It will also have the side effect of saving the guessed host to NSUserDefaults.

I think a better fix would be to use packagerServerHost from RCTPackagerConnection instead of jsLocation. packagerServerHost will need to be added to RCTBundleURLProvider header in dev only (like isPackagerRunning).

[reyalpsirc]

@janicduplessis I didn't notice that it would saved it to NSUserDefaults. I just made packagerServerHost accessible and changed RCTPackagerConnection so that i uses it instead of jsLocation.

I did not make the access to packagerServerHost for Dev only because RCTBundleURLProvider uses it internally even for non Dev scenarios afaik (and packagerServerHost always returns the jsLocation for non Dev cases).

[janicduplessis]

Thanks, LGTM!

ping @TheSavior

[facebook-github-bot]

@TheSavior has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[elicwhite]

This has failing unit tests:

Failing tests:
	RNTesterUnitTests:
		-[RCTBundleURLProviderTests testBundleURL]

While you fix this test can you make sure to add any additional tests that are needed to cover this change?

[reyalpsirc]

@TheSavior I lack the experience with Unit Tests of such kind, specially on Obj C, which I know very little.

How exactly can I check the failing test? Because the code I made was built directly on the react-native code that I have on a project and then moved to here once it worked well through the build.

[janicduplessis]

@reyalpsirc You can open RNTester/RNTesterPods.xcworkspace in xcode (make sure to also run pod install in the RNTester folder before).

Then you can run tests there:

[janicduplessis]

@TheSavior do you have any more log about the failure? Weird because it passed OSS CI.

RCTBundleURLProviderTests
    ✓ testBundleURL (0.042 seconds)
    ✓ testIPURL (0.004 seconds)
    ✓ testLocalhostURL (0.004 seconds)

[elicwhite]

Test Case '-[RCTBundleURLProviderTests testBundleURL]' started.
2020-04-02 10:54:52.894265-0700 xctest[75291:47235943] Task <5CA7F9B8-D968-43A2-8417-F1D728AE77EE>.<1> finished with error [-1001] Error Domain=NSURLErrorDomain Code=-1001 "The request timed out." UserInfo={NSUnderlyingError=0x7fcfbfea8170 {Error Domain=kCFErrorDomainCFNetwork Code=-1001 "(null)" UserInfo={_kCFStreamErrorCodeKey=-2102, _kCFStreamErrorDomainKey=4}}, NSErrorFailingURLStringKey=http://localhost:8081/status, NSErrorFailingURLKey=http://localhost:8081/status, _kCFStreamErrorDomainKey=4, _kCFStreamErrorCodeKey=-2102, NSLocalizedDescription=The request timed out.}
/data/sandcastle/boxes/trunk-hg-fbobjc-fbsource/xplat/js/react-native-github/RNTester/RNTesterUnitTests/RCTBundleURLProviderTests.m:75: error: -[RCTBundleURLProviderTests testBundleURL] : ((URL) equal to (localhostBundleURL())) failed: ("file:///usr/local/fbprojects/packages/xcode/149/xcode_11.3.0.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/Library/Xcode/Agents/main.jsbundle") is not equal to ("http://localhost:8081/test.jsbundle.bundle?platform=ios&dev=true&minify=false")
2020-04-02 10:54:52.896288-0700 xctest[75291:47236516] Connection 1: unable to determine interface type without an established connection
2020-04-02 10:54:52.896534-0700 xctest[75291:47236516] Connection 1: unable to determine fallback status without a connection
Test Case '-[RCTBundleURLProviderTests testBundleURL]' failed (60.545 seconds).
2020-04-02 10:54:52.901449-0700 xctest[75291:47236516] Task <5CA7F9B8-D968-43A2-8417-F1D728AE77EE>.<1> HTTP load failed, 0/0 bytes (error code: -999 [1:89])
Test Case '-[RCTBundleURLProviderTests testIPURL]' started.
Test Case '-[RCTBundleURLProviderTests testIPURL]' passed (0.008 seconds).
Test Case '-[RCTBundleURLProviderTests testLocalhostURL]' started.
Test Case '-[RCTBundleURLProviderTests testLocalhostURL]' passed (0.007 seconds).
Test Suite 'RCTBundleURLProviderTests' failed at 2020-04-02 10:54:52.919.

Ah. My guess is that since we run all of the tests without a network condition, something changed here causing it to try to make a network request. I'll retry the job to validate that it wasn't just a flake. Is there something in this test that was mocking this out before that isn't being used with now with this change?

[reyalpsirc]

@TheSavior I checked the test that you mentioned on my side and it seemed to work:

Also, correct me if I'm wrong but I think that it's not possible to build a test for this because it targets builds made for devices, which means that Automated tools like Circle CI would fail because they do not run these agains real devices?
And when I tried to run the tests on my iPad, XCode told me that "Logic Testing on iOS devices is not supported" which means that it's indeed not possible to make specific tests for devices in order to test the websocket connection between Metro and the device which is used to issue the "r" key reload command?

[react-native-bot]

This pull request was successfully merged by @reyalpsirc in f9df933.

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

[elicwhite]

Apparently it passed when I re ran it

[janicduplessis]

👍 Cool, found it weird that it failed since the PR doesn't actually touch the code that was tested by the failing test anymore (it did in the first version of the PR tho).