Fix editor launch freeze in debug mode when packager isn't running

[ceyhun]

Fixes #2944
Gutenberg PR: WordPress/gutenberg#29859
WPiOS PR: wordpress-mobile/WordPress-iOS#16094

I was able to reproduce this on a device. I observed that it only happens when you open the editor the first time and there's no delay on subsequent openings for me. Could be related to different network setups. I was able to fix the delay on the first run for my case, but can't know for sure if this will work in general.

The reason was that the jsBundleURL method, which calls a function that checks dev machine's local IP for a running server was being called multiple times during the editor launch:
1- Initializing the bridge in isPackagerRunning => delegate owner calls it
2- Getting the sourceURL in isPackagerRunning => we call it

Currently the lazy initialization of the bridge happens on this line: let url = sourceURL(for: bridge), along with the sourceURL call. So one line calls it twice :)

I removed this and now only the bridge is lazily initialized inside the isLoaded method when it is accessed the first time and the jsBundleURL is called once. This resolved the delay for me, but as a side effect when the editor is launched there's more verbose logging from the JS side to the Xcode console.

To get around this, I first thought of eagerly initializing the bridge in Gutenberg class' init method and checking the bundleURL property of the bridge in the isPackagerRunning method. But RCTBridge's init method has a reference to self as it's delegate, so this can't be done on init and we need to change the delegate to be another class instead. I thought in the end this might require a lot of changes and chose to keep the verbose log during debug by removing isPackagerRunning and logThreshold completely. Now the logLevel is set according to the DEBUG flag and is the same whether the packager is running or not.

The default setting for this in React Native is in RCTLog.mm:

#if RCT_DEBUG
static const RCTLogLevel RCTDefaultLogThreshold = (RCTLogLevel)(RCTLogLevelInfo - 1);
#else
static const RCTLogLevel RCTDefaultLogThreshold = RCTLogLevelError;
#endif

If a dev prefers the trace level threshold instead of info during debugging like it was before when a packager was detected, this can now be done manually in code by calling RCTSetLogThreshold(.trace) in the initializer. But I feel like this shouldn't be the case because it's only one level below and also I couldn't find a call to RCTLogTrace macro anywhere.

Feel free to give this a spin and let me know if it fixes the delay for you too.

PR submission checklist:

• I have considered adding unit tests where possible.
• I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

[guarani]

I was able to reproduce this on a device. I observed that it only happens when you open the editor the first time and there's no delay on subsequent openings for me. Could be related to different network setups.

Just noting that I wasn't able to reproduce this issue today (on develop) until I turned off my device WiFi. I'll add this to the issue's steps to reproduce, but please let me know if you see this issue with WiFi on (i.e. connected to the same network as your computer).

[ceyhun]

Just noting that I wasn't able to reproduce this issue today (on develop) until I turned off my device WiFi. I'll add this to the issue's steps to reproduce, but please let me know if you see this issue with WiFi on (i.e. connected to the same network as your computer).

On this branch I tried with Wifi on and was able to reproduce. But now I tried on develop and it seems to work fine both with wifi on and off.

[chipsnyder]

Hey @ceyhun, 👋 I'm working on cutting the 1.49.0 release, so I am bumping this out of 1.49.0 and bumped it to 1.50.0
Let me know if you think we should work to include this in 1.49.0 instead. Thanks!

[guarani]

I'd tried this a few days ago and it seemed to be working, but I tried again today and I'm still getting the freeze for 1 minute when running the code from the linked WPiOS PR. Here's my setup:

1. I checkout the WPiOS branch (wordpress-mobile/WordPress-iOS@0b6371f)
2. Run rake xcode
3. In Xcode, with a physical device connected via cable, I run the project (making sure I had no packagers running)
4. My device was on a data connection (WiFi off)
5. I open a new post on any of a number of sites and the app freezes for one minute before opening the block editor

The difference I notice, which you mentioned above, is that now when I open the editor, the Xcode log is much more verbose and includes fairly huge chucks of JSON that were not being displayed before IIRC.

Is your setup the same? I'm trying to figure out what accounts for the differences between what we see.

[ceyhun]

4. My device was on a data connection (WiFi off)

Oh, I wasn't testing with cellular data when Wifi was off, the phone I was testing with didn't have a sim card. Now I tried inserting one and I can reproduce on cellular data, even on this branch!

The freeze still seems to happen here on line 94:

https://github.com/facebook/react-native/blob/b9944e54ae35c2beed0e78ea454d871e0fe92ec6/React/Base/RCTBundleURLProvider.m#L76-L98

But with that information, I think it makes sense to skip isPackagerRunning directly on cellular. So for a fix we can keep the changes here and on top of these add something like this in sourceURL method:

#if DEBUG
if isOnCellularNetwork {
    return Bundle.main.url(forResource: "main", withExtension: "jsbundle")
}
#endif

Not sure how to implement the isOnCellularNetwork part yet though :)

[guarani]

I agree it makes sense that if WiFi is OFF and cellular data is ON, the app should use the prepackaged bundle instead of loading from the network.

But with that information, I think it makes sense to skip isPackagerRunning directly on cellular.

I'm not sure I follow, your current changes already removed isPackagerRunning altogether. Do you mean on cellular we should skip jsBundleURL instead here?

Not sure how to implement the isOnCellularNetwork part yet though :)

We could use a lib like Reachability, I don't know of an easy way to do this without a library either.

[ceyhun]

I'm not sure I follow, your current changes already removed isPackagerRunning altogether. Do you mean on cellular we should skip jsBundleURL instead here?

Oh sorry about that, there's another isPackagerRunning inside react-native here. But yeah, what I meant was skipping jsBundleURL which is calling RN's isPackagerRunning eventually.

We could use a lib like Reachability, I don't know of an easy way to do this without a library either.

That could mean adding that library to Gutenberg pod as a dependency, only to be used in debug mode of WPiOS :( I just found out about NWPathMonitor which is available on iOS 12.0+, maybe it's time for me to revisit this PR to bump deployment target to iOS 13 so we can use that :D

[guarani]

Nice, TIL about NWPathMonitor 👍 That sounds like a good plan. If it doesn't work though, adding a pod as a debug dependency should be possible with:

pod 'Reachability', :configurations => ['Debug']

[peril-wordpress-mobile]

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

[ceyhun]

@guarani Implemented a solution with NWPathMonitor, can you please give this another look :)

[jd-alexander]

@ceyhun @guarani Bumping the milestone to 1.51, feel free to ping me if it needs to make it to 1.50 this week.

[guarani]

Tested on a real device on both data and WiFi connections and it worked as expected. I also tested to ensure the packager if running, is connected to successfully and also OK. Did a quick check on a simulator and it's also working great!
Thanks a lot @ceyhun! I think this can be merged to 1.50 if you'd like.

[jd-alexander]

I think this can be merged to 1.50 if you'd like

Updated milestone to reflect this!

[ceyhun]

Thanks a lot @guarani! Merged the gutenberg PR already and updated the gutenberg reference to current trunk here, just a heads-up a few more changes could be included in 1.50 coming in from trunk.
Will press this Enable auto-merge button while the CI keeps running, let's see how it goes 😄