[ios][android] Make redbox and rr (android) and cmd+r (ios) reload manifest and JS rather than just JS #6134

brentvatne · 2019-10-30T18:09:10Z

Why

It is confusing when you reload your app and the properties you changed in the manifest are not applied.

Now that we are getting rid of "Reload JS" and making of the distinction between "Reload Manifest and JS" and just "Reload" in the developer menu on iOS (#6110), I want to also get rid of the "Reload JS" behavior when you hit the reload hotkeys in the simulator/emulator.

More context on reloading changes in this public doc: https://www.notion.so/expo/Reloading-behavior-4be3c92671744110b93d3190c85d13f4

How

iOS: call refreshVisibleApp on browser control rather than reloadBridge on app manager when hotkey is pressed. override reload block in redbox.
Android: reloadExpoApp in DevSupportManagerImpl (and DevSupportManager) which calls reloadFromManifest with activity id on ReactNativeStaticHelpers,, which uses reflection to call reloadVisibleExperience on the kernel. Update locations where we reload when DoubleTapReloadRecognizer is used to call reloadExpoApp instead of reloadJS on the DevSupportManager implementation, and make the reload button in redbox do the same.

Test Plan

Use these hotkeys in simulator/emulator, try red box.

Implementation Notes

I'm not crazy about the Android implementation here. In the future it would be nice to:

Change the dev menu on Android to be similar to our iOS dev menu, rather than forking the React Native dev menu on each release.
Possibly get rid of the ability to open multiple projects from Expo client on Android simultaneously. The iOS client only handles one at a time, and I don't know if the extra complexity in terms of code and user-facing behavior is worth it.
Remove the "pin" and "share" buttons from the notification drawer and just leave reload in place.

Concerns

Are there some edge cases on Android where reloading won't work as expected?
Was it too early for upstream to remove live reload? There are some cases where fast refresh doesn't work perfectly yet, such as when changing the static options in react-navigation. How can we educate users about such limitations? Also, fast refresh doesn't work when prod mode is enabled and so people need to refresh manually in that situation.

esamelson

Android side of this looks fine to me (so much reflection 😶 ). Would be good to update this in react-native-lab as well -- unlike with iOS, the ReactAndroid folder is copied rather than symlinked so this change wouldn't be persisted next SDK version. But we should definitely just make our own dev menu in the future so we don't have to modify the RN internals as much.

esamelson · 2019-11-06T01:40:25Z

android/expoview/src/main/java/host/exp/exponent/kernel/Kernel.java

@@ -741,6 +741,26 @@ public void onEventFailure(String errorMessage) {
    killOrphanedLauncherActivities();
  }

+  public static void reloadVisibleExperience(final int activityId) {


it looks like this needs a @DoNotStrip, and maybe the method below does not (since it's called from this one)

esamelson · 2019-11-06T01:42:13Z

android/expoview/src/main/java/host/exp/exponent/ReactNativeStaticHelpers.java

+  }
+
+  @DoNotStrip
+  public static String getManifestUrlForActivityId(final int activityId) {


is this ever called?

it's not. i originally planned on using it but didn't, i thought i'd leave it in as someone might want it in the future but on second thought i'll just remove it

esamelson · 2019-11-06T01:43:54Z

android/expoview/src/main/java/host/exp/exponent/ReactNativeStaticHelpers.java

+  @DoNotStrip
+  public static void reloadFromManifest(final int activityId) {
+    try {
+      Class.forName("host.exp.exponent.kernel.Kernel")


i'm confused about why the methods in this class are all using reflection to call static methods on Kernel (which is in the same package...) rather than just calling the methods directly. but maybe best to just leave it alone at this point 🤷‍♂

yeah i'm not sure either but i just did what other things in here were doing

brentvatne · 2019-11-07T04:46:47Z

Would be good to update this in react-native-lab as well -- unlike with iOS, the ReactAndroid folder is copied rather than symlinked so this change wouldn't be persisted next SDK version

maybe we can chat briefly about the process here tmrw - i went to update everything in react-native-lab and found that there was already a large diff before my changes, eg: DevSuportManagerImpl.java on master of expo/expo and on react-native-lab are very different.

…side of developer mode

ide · 2019-11-08T04:04:28Z

++ to consistent refresh behavior. Are there any docs or old but popular forum posts about the refresh behavior we need to update? (If there are none, I don't think we need to write any, since this new behavior is closer to how things should be, subjectively speaking.)

brentvatne · 2019-11-14T18:18:46Z

i don't believe that there are any, but the 'development mode' docs will need to be updated for the changes to the dev menu cc @cruzach who is already aware of this

…po and :tools:execute Squashed three commits: - d8f3fbe - ae5e396 - 2217620 The first one introduced breaking changes for bare workflow in our React Native fork, the second one fixed them by integrating with ReactAndroidCodeTransformer used when we import the code into Expo Client, the third one added usage for `reloadExpoApp` to debugger UI. Co-Authored-By: brentvatne <brentvatne@gmail.com> Co-Authored-By: esamelson <eric@expo.io> Co-Authored-By: Kudo Chien <kudo@expo.dev>

brentvatne force-pushed the @brent/always-reload-from-manifest branch from e3bc833 to 8bc468d Compare November 2, 2019 00:53

brentvatne mentioned this pull request Nov 2, 2019

Can't escape "Production Mode" when developing expo/expo-cli#1168

Closed

brentvatne force-pushed the @brent/always-reload-from-manifest branch from ed9e723 to 7a5d2fc Compare November 2, 2019 21:38

brentvatne marked this pull request as ready for review November 2, 2019 21:47

brentvatne changed the title ~~[ios][android] Make rr (android) and cmd+r (ios) reload manifest and JS rather than just JS~~ [ios][android] Make redbox and rr (android) and cmd+r (ios) reload manifest and JS rather than just JS Nov 2, 2019

brentvatne requested review from esamelson and ide November 5, 2019 18:40

esamelson reviewed Nov 6, 2019

View reviewed changes

sjchmiela added the Launch-blocking label Nov 7, 2019

brentvatne added 7 commits November 7, 2019 11:08

[ios] Make cmd+r reload manifest and JS rather than just JS

1c850b1

[ios] Reload from dev menu grabs manifest too

e8bd459

[ios] Clean up reload code and ensure redbox injection code works out…

ae6254f

…side of developer mode

[android] Reload on dev menu also reloads from manifest

37f13f5

[ios] Fix typo

50dcf6d

[android] Reload from manifest from redbox and with rr shortcut

c7e1269

[android] Address feedback

581c966

brentvatne force-pushed the @brent/always-reload-from-manifest branch from 7a5d2fc to 581c966 Compare November 7, 2019 19:08

brentvatne added a commit to expo/react-native that referenced this pull request Nov 7, 2019

[android] Always reload from manifest expo/expo#6134

27f4c90

brentvatne added 2 commits November 7, 2019 11:12

[react-native-lab] Copy changes to ReactAndroid over

5e7a85f

[android] Clean up extra newlines

f0fff43

brentvatne added a commit to expo/react-native that referenced this pull request Nov 8, 2019

[android] Always reload from manifest expo/expo#6134

f3e8942

[react-native-lab] Update on sdk-36 branch

b4d32a1

brentvatne merged commit b333b41 into master Nov 8, 2019

brentvatne deleted the @brent/always-reload-from-manifest branch November 8, 2019 02:41

sjchmiela pushed a commit to expo/react-native that referenced this pull request Nov 12, 2019

[android] Always reload from manifest expo/expo#6134

dce05e8

sjchmiela pushed a commit to expo/react-native that referenced this pull request Nov 13, 2019

[android] Always reload from manifest expo/expo#6134

2a2663f

sjchmiela pushed a commit to expo/react-native that referenced this pull request Nov 15, 2019

[android] Always reload from manifest expo/expo#6134

c492973

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ios][android] Make redbox and rr (android) and cmd+r (ios) reload manifest and JS rather than just JS #6134

[ios][android] Make redbox and rr (android) and cmd+r (ios) reload manifest and JS rather than just JS #6134

Uh oh!

brentvatne commented Oct 30, 2019 •

edited

Loading

Uh oh!

esamelson left a comment

Uh oh!

esamelson Nov 6, 2019

Uh oh!

esamelson Nov 6, 2019

Uh oh!

brentvatne Nov 7, 2019

Uh oh!

esamelson Nov 6, 2019 •

edited

Loading

Uh oh!

brentvatne Nov 7, 2019

Uh oh!

brentvatne commented Nov 7, 2019

Uh oh!

ide commented Nov 8, 2019

Uh oh!

brentvatne commented Nov 14, 2019

Uh oh!

Uh oh!

[ios][android] Make redbox and rr (android) and cmd+r (ios) reload manifest and JS rather than just JS #6134

[ios][android] Make redbox and rr (android) and cmd+r (ios) reload manifest and JS rather than just JS #6134

Uh oh!

Conversation

brentvatne commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

How

Test Plan

Implementation Notes

Concerns

Uh oh!

esamelson left a comment

Choose a reason for hiding this comment

Uh oh!

esamelson Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

esamelson Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

brentvatne Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

esamelson Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brentvatne Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

brentvatne commented Nov 7, 2019

Uh oh!

ide commented Nov 8, 2019

Uh oh!

brentvatne commented Nov 14, 2019

Uh oh!

Uh oh!

brentvatne commented Oct 30, 2019 •

edited

Loading

esamelson Nov 6, 2019 •

edited

Loading