fix: [iOS] RCTReloadCommand should pass its bundleURL when reloading

[douglowder]

Summary:

An Expo feature that makes use of the RCTReloadCommand class RCTTriggerReloadCommandListeners() to reload with a specific bundle URL is not working, because setting the bundle URL via RCTReloadCommandSetBundleURL() has no effect.

The fix is to add a new method to the RCTReloadListener protocol that allows the new URL to be passed into the listener (RCTBridge or RCTHost) to set the bundle URL correctly before reload.

Changelog:

[iOS][Fixed] RCTReloadCommand should pass its bundle URL when reloading

Test Plan:

Working on a non-Expo test app to demonstrate the fix.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	19,387,939	-11
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	22,761,791	-5
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 2509eb7
Branch: main

[motiz88]

Hey @douglowder! Can you please elaborate on the Expo feature and why it needs to set a custom bundle URL?

[douglowder]

This is for the expo-updates package. There is an API that allows a downloaded update to be immediately launched, replacing the currently running JS bundle.

[douglowder]

I should add that this was working in 0.73, and still works in 0.74 on Android, so this may actually be a regression.

[cipolleschi]

Hi @douglowder, we fixed something similar in Bridgeless mode, when the bundleURL was not recomputed. Also, in most case, the bundleURL is stored as a variable when React Native starts and it's never re-evaluated. If, upon reload, you'd like to change the url, I don't think this solution will work.

How are you testing it and on which react native versions? Can you test with the latest RC?

[douglowder]

@cipolleschi I'm testing it with 0.74.0-rc.9, in https://github.com/expo/UpdatesAPIDemo/tree/canary .

It seems to work as expected for both non-Fabric and bridgeless.

[cipolleschi]

Can you elaborate more what controls how the bundleURL changes?

If the bundleURL is dynamic across reloads, my approach would be to change the bundleURL property to a closure that can be re-evaluated as needed.
This is less error prone than managing manually the bundleURL state, which can get stale.

What do think about this?

Edit:
Also, can you clarify the following:

1. Before your fix, was it broken in 0.74.0-RC.9, with the Old Architecture?
2. Before your fix, was it broken in 0.74.0-RC.9, with the New Architecture, Bridge Mode?

[douglowder]

@cipolleschi the bundleURL only changes if a user calls our JS API to reload the app. If a newer bundle has been downloaded, the file URL for that bundle becomes the new bundleURL.

Before the fix in this PR, the behavior is broken for us in both old architecture and new architecture bridgeless. We have not been testing with new architecture and bridge.

Edit: if bundleURL should not change, what is the purpose of the RCTReloadCommandSetBundleURL() method?

[cipolleschi]

From what I can see, the RCTReloadCommandSetBundleURL is only used in the framework when the Bridge or the Host are created, so it is not invoked further down when the app is already running. It is either a legacy of the past or a way to communicate the url to the RCTReloadCommand.

That said, I'm wondering what changed in the old architecture that broke it between 0.73 and 0.74. The regression must be fixed, but rather than adding new functions and codepath we need to maintain, it would be better to understand first what broke it.

I'm not pushing back on the fix, to make it clear, but we do have to understand better what stopped working to find the best fix.

Do you think you can put together a repro using this template so we can investigate the issue better?

---

I think I know what happened.

With this change, we changed the way the bundleURL is handled.

Before, we were asking the AppDelegate for the url and we were storing the url in a variable. This could lead to scenarios where the bundleURL was stale and never updated, especially in bridgeless mode. Which was the problem that #43994 was fixing by passing a closure that could dynamically update the value of the URL.

With that change, the reload command was changed so that, upon reload, it was re-evaluating the closure and thus getting a new url.
The stored variable is therefore not used anymore.

What I think Expo was doing is to leverage that "staleness" of the variable to update it as it needed. But that's not something reliable, and in fact, we changed that.

The way apps should provide the bundle URL is through the - (NSURL *)bundleURL function in the AppDelegate. Do you think it would be possible to fix this on Expo side, rather then modifying the framework, so that this function returns the right bundle url when a user calls Expo's JS API to reload the app?

[douglowder]

@cipolleschi thanks for researching this and providing a detailed analysis!

Per your advice, we will close this and fix the issue on the Expo side.