[Workflow][Android] Provide an option to never load JS from cache

[ide]

Currently RN Android loads JS from a cache if it can't reach the bundle server. This is for teams working on hybrid apps since not everyone is running a packager. But when you're doing RN development you want to always be loading the most recent version of the code, so some option to never load from cache would be good.

[Wingie]

I'd like to take this up, (but i'm just starting out and would like some guidance)
i'm facing #2747 and i think this might be related to it.. would like to do some investigating.

i've just started diving into the code. i think the relevant code is handled by JSBundleLoader's createCachedBundleFromNetworkLoader. looking further loadScriptFromNetworkCached seems to be a native jni method? as it does react::loadScriptFromFile to actually load the bundle.

There is a comment ... loader expect JS bundle to be prefetched and stored in local file. We do that to avoid passing large strings between java and native code and avoid allocating memory in java to fit whole JS bundle in it.

I had a doubt as to where is the prefetching actually happening? and can the application be notified that the bundle on file has changed so it can update itself.
i guess this is how the autoreload would be working, but for me if i enable that - its happening continuously i.e. even without me changing any index.android.js code; so i want to debug and see why that is happening.

[qbig]

/**
   * This loader is recommended one for release version of your app. In that case local JS executor
   * should be used. JS bundle will be read from assets directory in native code to save on passing
   * large strings from java to native memory.
   */
  public static JSBundleLoader createFileLoader(
      final Context context,
      final String fileName) {
    return new JSBundleLoader() {
      @Override
      public void loadScript(ReactBridge bridge) {
        if (fileName.startsWith("assets://")) {
          bridge.loadScriptFromAssets(context.getAssets(), fileName.replaceFirst("assets://", ""));
        } else {
          bridge.loadScriptFromFile(fileName, fileName);
        }
      }

      @Override
      public String getSourceUrl() {
        return fileName;
      }
    };
  }

  /**
   * This loader is used when bundle gets reloaded from dev server. In that case loader expect JS
   * bundle to be prefetched and stored in local file. We do that to avoid passing large strings
   * between java and native code and avoid allocating memory in java to fit whole JS bundle in it.
   * Providing correct {@param sourceURL} of downloaded bundle is required for JS stacktraces to
   * work correctly and allows for source maps to correctly symbolize those.
   */
  public static JSBundleLoader createCachedBundleFromNetworkLoader(
      final String sourceURL,
      final String cachedFileLocation) {
    return new JSBundleLoader() {
      @Override
      public void loadScript(ReactBridge bridge) {
        bridge.loadScriptFromFile(cachedFileLocation, sourceURL);
      }

      @Override
      public String getSourceUrl() {
        return sourceURL;
      }
    };
  }

  /**
   * This loader is used when proxy debugging is enabled. In that case there is no point in fetching
   * the bundle from device as remote executor will have to do it anyway.
   */
  public static JSBundleLoader createRemoteDebuggerBundleLoader(
      final String sourceURL) {
    return new JSBundleLoader() {
      @Override
      public void loadScript(ReactBridge bridge) {
        bridge.loadScriptFromFile(null, sourceURL);
      }

      @Override
      public String getSourceUrl() {
        return sourceURL;
      }
    };
  }

Hi @ide . Based on the code above, can we just add an checkbox in the dev popup menu, 'load from cached [x'] and use 'null' for cachedFileLocation when we set to 'never load from cache'?

[satya164]

@qbig Probably the checkbox should go into the developer settings screen. Up for sending a pull request? ;)

[qbig]

@satya164 yup:)

[qbig]

@ide @satya164 could you help me out with this ? Seems that if I just naively passing 'null' as 'cachedFileLocation', it won't work for JSCExecutor, while it's fine for ProxyExecutor. @ide You mentioned that if RN cannot reach the dev server, it would try to read from the bundle. Is that part of the cpp native code : ?

public native void loadScriptFromFile(@Nullable String fileName, @Nullable String sourceURL);

[qbig]

  /**
   * This loader is used when bundle gets reloaded from dev server. In that case loader expect JS
   * bundle to be prefetched and stored in local file. We do that to avoid passing large strings
   * between java and native code and avoid allocating memory in java to fit whole JS bundle in it.
   * Providing correct {@param sourceURL} of downloaded bundle is required for JS stacktraces to
   * work correctly and allows for source maps to correctly symbolize those.
   */
  public static JSBundleLoader createCachedBundleFromNetworkLoader(
      final String sourceURL,
      final String cachedFileLocation) {
    return new JSBundleLoader() {
      @Override
      public void loadScript(ReactBridge bridge) {
        bridge.loadScriptFromFile(cachedFileLocation, sourceURL);
      }

      @Override
      public String getSourceUrl() {
        return sourceURL;
      }
    };
  }

@ide
Does this piece of code has anything to do with 'if failed, load from cache' as you mentioned? The comment on top seems to be talking about something else.

[ide]

It's been awhile since I looked at the code but I think you were right that this logic may be in C++. There is a native JNI function that takes both a URL and file path and loads JS -- I believe that function may be loading the JS from the file path if the URL cannot be reached.

[qbig]

Sorry ..@ide I was totally wrong. It seems that it has nothing to do with the native cpp code.
in DevSupportManager.java

public boolean hasUpToDateJSBundleInCache() {
    if (mIsDevSupportEnabled && mDevServerHelper.getShouldUseCachedBundle() && mJSBundleTempFile.exists()) {
      try {
        String packageName = mApplicationContext.getPackageName();
        PackageInfo thisPackage = mApplicationContext.getPackageManager()
            .getPackageInfo(packageName, 0);
        if (mJSBundleTempFile.lastModified() > thisPackage.lastUpdateTime) {
          // Base APK has not been updated since we donwloaded JS, but if app is using exopackage
          // it may only be a single dex that has been updated. We check for exopackage dir update
          // time in that case.
          File exopackageDir = new File(
              String.format(Locale.US, EXOPACKAGE_LOCATION_FORMAT, packageName));
          if (exopackageDir.exists()) {
            return mJSBundleTempFile.lastModified() > exopackageDir.lastModified();
          }
          return true;
        }
      } catch (PackageManager.NameNotFoundException e) {
        // Ignore this error and just fallback to loading JS from assets
        FLog.e(ReactConstants.TAG, "DevSupport is unable to get current app info");
      }
    }
    return false;
  }

[qbig]

https://developer.apple.com/library/tvos/documentation/JavaScriptCore/Reference/JSBase_header_reference/index.html#//apple_ref/c/func/JSEvaluateScript

The redbox is because I attempted to passing null as cachedFileLocation, which is used for script, when passing to JSC. But that does not work because sourceURL is merely used for exceptions.

But it works for ProxyExecutor, as ProxyExecutor(Chrome) prefer * sourceURL* and don't require script at all.

So although the interface is the same, the 2 implementation of Executor behave quite different. On top of that, the redbox information is kinda misleading. So maybe another issue for that as well.

[mkonicek]

@ide What do you want the behavior to be if the packager is not running? Added comments to this PR: #4738

[ide]

@mkonicek my desired behavior (during development) is to show a redbox saying that we couldn't download the bundle. This way if the packager is running but your phone can't connect to it (for example, if you are on a different WiFi LAN) there is no chance of loading a cached bundle by accident.

For Facebook's case where not everyone is not programming with RN yet, I think what you want is a custom JS loader that checks a flag (ex: in shared preferences) for whether you are developing with RN or not. If the flag is false, then load a JS bundle published by a CI bot and enable bundle caching. If the flag is true, then always load from packager -- never cache.

[qbig]

#4738 tackle this by changing "Dev Setting“， are you suggesting to change the behaviour otherwise ? (set flag in MainActivity or allowing both ways to change the flag) @ide @mkonicek

[astreet]

Sorry, just saw this. I think we should get rid of the cached bundle thing on Android completely. We don't have it on iOS and it just makes all of this loading logic harder to follow and less consistent. I'll also comment on the PR. @kmagiera (who added this), do you disagree?

[kmagiera]

So loading from cache is not to support a hybrid app use-case but to actually help people testing RN changes on real devices (people that only work on native changes will always have empty cache as they usually don't have packager running).

I'm fine with getting rid of it. Actually prefer that to having this in a dev settings which:

• make the logic more complicated
• require extra documentation to make it discoverable
• you can easily forget to update your settings when you re-install app and then loose some time figuring out why the behaviour is different

What I think would be necessary in that case is to provide a clean and visible notification to the user for the cases when JS is loaded from the bundle (when packager is not reachable).

Think of a scenario when you test your app on real device. In that case when your app crashes (or you close it and re-open to test initial screen or sth) when you open the app again you expect for your app to run the newest version of the code. Unfortunately since "adb reverse" randomly "disconnects" quite often, without noticing you may end up running the outdated code that has been bundled together with your app.

[astreet]

@kmagiera: On the PR, @satya164 makes the good point (as you also mention above) that this feature is useful for testing on real devices without having to do a full install so I'm a bit less inclined to remove it now. I agree with your concerns around the setting, but don't see a great other option.

A notification about where the bundle is loaded from in dev mode seems like a reasonable compromise, but I'm not sure where it could go. The RN bundle is often loaded in the background before any RN UI is shown in our FB hybrid apps.

[satya164]

@astreet We can show a Android Toast/Snackbar on Android I guess?

[astreet]

imo that sounds pretty annoying to see on every load. What about changing the toolbar color?

[satya164]

@astreet yeah, it's a bit annoying. Or we could show only when we're loading from cache.

Which toolbar? If you meant statusbar, it won't work for my case at least since I configure the app to draw under statusbar.

[astreet]

Yeah sorry, status bar: http://developer.android.com/reference/android/view/Window.html#setStatusBarColor(int). This would only be something that's enabled in developer mode (and I think you can still make that color transluscent).

[satya164]

@astreet The docs say,

FLAG_TRANSLUCENT_STATUS must not be set

:(

[astreet]

Do we use that somewhere?

If you're just referring to me talking about translucent colors:

If color is not opaque, consider setting SYSTEM_UI_FLAG_LAYOUT_STABLE and SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN.

so it sounds like they support translucent colors in the status bar

[satya164]

@astreet I was talking about FLAG_TRANSLUCENT_STATUS. We're not using it anywhere in react Native. But I use in my project, and I suspect other people might be using it too.

But I think > 99% of people might not be using it. So an acceptable compromise.

[astreet]

Hmm... ok. I'm not sure the color is a good idea anyway -- it won't be clear what it means. Let's try experimenting with the Snackbar and see how that feels.

[jsierles]

To keep issues lean and focused on bugs, I'm closing this out in favor of the discussion in #4738. Thanks!