From 89d72c99be8954976c0681b780bf32d4583a754f Mon Sep 17 00:00:00 2001 From: Ashok Menon Date: Fri, 13 Jan 2017 03:52:07 -0800 Subject: [PATCH] BREAKING - (Re)moving `JSBundleLoader.getSourceUrl()` Summary: == What == Changing the `JSBundleLoader` API, to remove `String getSourceUrl()`, instead `JSBundleLoader.loadScript` now returns the source URL it loaded. This change has a knock-on effect: We can no longer populate `SourceCodeModule` when we construct it, because at that time, we do not know the source URL. In order to solve this I have made the following changes: - Added `CatalystInstance.getSourceURL()`, to return the source URL from the instance after the JS Bundle has been loaded, or `null` otherwise. - Removed `ReactInstanceManager.getSourceUrl()`, because its only purpose was to populate `SourceCodeModule`. - Also removed `ReactInstanceManager.getJSBundleFile()` because it was only being used in a test confirming that the `ReactInstanceManager` knew its bundle file as soon as it was constructed, which is no longer necessarily true. - Initialise `SourceCodeModule` with the `ReactContext` instance it belongs to. - Override `NativeModule.initialize()` in `SourceCodeModule` to fetch the source URL. When the `SourceCodeModule` is constructed, the context does not have a properly initialised `CatalystInstance`, but by the time we call initialise on it, the `ReactContext` has a `CatalystInstance` and that in turn has a source URL. == Why == The reason for this change is that it allows us to add implementations of `JSBundleLoader`, that cannot determine their source URL until after having performed a load successfully. In particular I plan to introduce `FallbackJSBundleLoader` which will try to load from multiple sources in sequence stopping after the first successful load. As load failures could happen for a variety of reasons, we can't know what the true source URL is without performing the load. Reviewed By: javache Differential Revision: D4398956 fbshipit-source-id: 51ff4e289c8723e9d242f23267181c775a6abe6f --- .../facebook/react/CoreModulesPackage.java | 2 +- .../facebook/react/ReactInstanceManager.java | 10 ------ .../react/XReactInstanceManagerImpl.java | 18 ----------- .../react/bridge/CatalystInstance.java | 9 ++++++ .../react/cxxbridge/CatalystInstanceImpl.java | 9 +++++- .../react/cxxbridge/JSBundleLoader.java | 32 ++++++------------- .../cxxbridge/OptimizedJSBundleLoader.java | 6 +--- .../react/modules/debug/SourceCodeModule.java | 19 +++++++++-- 8 files changed, 44 insertions(+), 61 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/CoreModulesPackage.java b/ReactAndroid/src/main/java/com/facebook/react/CoreModulesPackage.java index ce45cbe5e..acc5ef4db 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/CoreModulesPackage.java +++ b/ReactAndroid/src/main/java/com/facebook/react/CoreModulesPackage.java @@ -128,7 +128,7 @@ import static com.facebook.react.bridge.ReactMarkerConstants.CREATE_UI_MANAGER_M new ModuleSpec(SourceCodeModule.class, new Provider() { @Override public NativeModule get() { - return new SourceCodeModule(mReactInstanceManager.getSourceUrl()); + return new SourceCodeModule(reactContext); } })); moduleSpecList.add( diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java index b2e7285c9..517ffb464 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java @@ -154,16 +154,6 @@ public abstract class ReactInstanceManager { Intent data); public abstract void showDevOptionsDialog(); - /** - * Get the URL where the last bundle was loaded from. - */ - public abstract String getSourceUrl(); - - /** - * The JS Bundle file that this Instance Manager was constructed with. - */ - public abstract @Nullable String getJSBundleFile(); - /** * Attach given {@param rootView} to a catalyst instance manager and start JS application using * JS module provided by {@link ReactRootView#getJSModuleName}. If the react context is currently diff --git a/ReactAndroid/src/main/java/com/facebook/react/XReactInstanceManagerImpl.java b/ReactAndroid/src/main/java/com/facebook/react/XReactInstanceManagerImpl.java index d7c85c712..f798af3ed 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/XReactInstanceManagerImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/XReactInstanceManagerImpl.java @@ -127,7 +127,6 @@ import static com.facebook.systrace.Systrace.TRACE_TAG_REACT_JAVA_BRIDGE; private @Nullable volatile ReactContext mCurrentReactContext; private final Context mApplicationContext; private @Nullable DefaultHardwareBackBtnHandler mDefaultBackButtonImpl; - private String mSourceUrl; private @Nullable Activity mCurrentActivity; private final Collection mReactInstanceEventListeners = Collections.synchronizedSet(new HashSet()); @@ -634,22 +633,6 @@ import static com.facebook.systrace.Systrace.TRACE_TAG_REACT_JAVA_BRIDGE; mDevSupportManager.showDevOptionsDialog(); } - /** - * Get the URL where the last bundle was loaded from. - */ - @Override - public String getSourceUrl() { - return Assertions.assertNotNull(mSourceUrl); - } - - @Override - public @Nullable String getJSBundleFile() { - if (mBundleLoader == null) { - return null; - } - return mBundleLoader.getSourceUrl(); - } - /** * Attach given {@param rootView} to a catalyst instance manager and start JS application using * JS module provided by {@link ReactRootView#getJSModuleName}. If the react context is currently @@ -843,7 +826,6 @@ import static com.facebook.systrace.Systrace.TRACE_TAG_REACT_JAVA_BRIDGE; JSBundleLoader jsBundleLoader) { FLog.i(ReactConstants.TAG, "Creating react context."); ReactMarker.logMarker(CREATE_REACT_CONTEXT_START); - mSourceUrl = jsBundleLoader.getSourceUrl(); List moduleSpecs = new ArrayList<>(); Map reactModuleInfoMap = new HashMap<>(); JavaScriptModuleRegistry.Builder jsModulesBuilder = new JavaScriptModuleRegistry.Builder(); diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java index a6ab8fd65..725182227 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java @@ -9,6 +9,8 @@ package com.facebook.react.bridge; +import javax.annotation.Nullable; + import java.util.Collection; import com.facebook.proguard.annotations.DoNotStrip; @@ -23,6 +25,13 @@ import com.facebook.react.common.annotations.VisibleForTesting; @DoNotStrip public interface CatalystInstance extends MemoryPressureListener { void runJSBundle(); + + /** + * Return the source URL of the JS Bundle that was run, or {@code null} if no JS + * bundle has been run yet. + */ + @Nullable String getSourceURL(); + // This is called from java code, so it won't be stripped anyway, but proguard will rename it, // which this prevents. @DoNotStrip diff --git a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CatalystInstanceImpl.java b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CatalystInstanceImpl.java index 25985b666..9f4499198 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CatalystInstanceImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/CatalystInstanceImpl.java @@ -97,6 +97,7 @@ public class CatalystInstanceImpl implements CatalystInstance { private volatile boolean mAcceptCalls = false; private boolean mJSBundleHasLoaded; + private @Nullable String mSourceURL; // C++ parts private final HybridData mHybridData; @@ -188,8 +189,9 @@ public class CatalystInstanceImpl implements CatalystInstance { public void runJSBundle() { Assertions.assertCondition(!mJSBundleHasLoaded, "JS bundle was already loaded!"); mJSBundleHasLoaded = true; + // incrementPendingJSCalls(); - mJSBundleLoader.loadScript(CatalystInstanceImpl.this); + mSourceURL = mJSBundleLoader.loadScript(CatalystInstanceImpl.this); synchronized (mJSCallsPendingInitLock) { // Loading the bundle is queued on the JS thread, but may not have @@ -208,6 +210,11 @@ public class CatalystInstanceImpl implements CatalystInstance { Systrace.registerListener(mTraceListener); } + @Override + public @Nullable String getSourceURL() { + return mSourceURL; + } + private native void callJSFunction( ExecutorToken token, String module, diff --git a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JSBundleLoader.java b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JSBundleLoader.java index f0b813052..629a1d381 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JSBundleLoader.java +++ b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/JSBundleLoader.java @@ -29,12 +29,8 @@ public abstract class JSBundleLoader { final String assetUrl) { return new JSBundleLoader() { @Override - public void loadScript(CatalystInstanceImpl instance) { + public String loadScript(CatalystInstanceImpl instance) { instance.loadScriptFromAssets(context.getAssets(), assetUrl); - } - - @Override - public String getSourceUrl() { return assetUrl; } }; @@ -47,12 +43,8 @@ public abstract class JSBundleLoader { public static JSBundleLoader createFileLoader(final String fileName) { return new JSBundleLoader() { @Override - public void loadScript(CatalystInstanceImpl instance) { + public String loadScript(CatalystInstanceImpl instance) { instance.loadScriptFromFile(fileName, fileName); - } - - @Override - public String getSourceUrl() { return fileName; } }; @@ -70,18 +62,14 @@ public abstract class JSBundleLoader { final String cachedFileLocation) { return new JSBundleLoader() { @Override - public void loadScript(CatalystInstanceImpl instance) { + public String loadScript(CatalystInstanceImpl instance) { try { instance.loadScriptFromFile(cachedFileLocation, sourceURL); + return sourceURL; } catch (Exception e) { throw DebugServerException.makeGeneric(e.getMessage(), e); } } - - @Override - public String getSourceUrl() { - return sourceURL; - } }; } @@ -94,17 +82,15 @@ public abstract class JSBundleLoader { final String realSourceURL) { return new JSBundleLoader() { @Override - public void loadScript(CatalystInstanceImpl instance) { + public String loadScript(CatalystInstanceImpl instance) { instance.loadScriptFromFile(null, proxySourceURL); - } - - @Override - public String getSourceUrl() { return realSourceURL; } }; } - public abstract void loadScript(CatalystInstanceImpl instance); - public abstract String getSourceUrl(); + /** + * Loads the script, returning the URL of the source it loaded. + */ + public abstract String loadScript(CatalystInstanceImpl instance); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/OptimizedJSBundleLoader.java b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/OptimizedJSBundleLoader.java index 6efcb3a3f..96c07735b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/OptimizedJSBundleLoader.java +++ b/ReactAndroid/src/main/java/com/facebook/react/cxxbridge/OptimizedJSBundleLoader.java @@ -24,12 +24,8 @@ public class OptimizedJSBundleLoader extends JSBundleLoader { } @Override - public void loadScript(CatalystInstanceImpl instance) { + public String loadScript(CatalystInstanceImpl instance) { instance.loadScriptFromOptimizedBundle(mPath, mSourceURL, mLoadFlags); - } - - @Override - public String getSourceUrl() { return mSourceURL; } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/modules/debug/SourceCodeModule.java b/ReactAndroid/src/main/java/com/facebook/react/modules/debug/SourceCodeModule.java index 99752c88b..1023d8e3d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/modules/debug/SourceCodeModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/modules/debug/SourceCodeModule.java @@ -14,7 +14,9 @@ import javax.annotation.Nullable; import java.util.HashMap; import java.util.Map; +import com.facebook.infer.annotation.Assertions; import com.facebook.react.bridge.BaseJavaModule; +import com.facebook.react.bridge.ReactContext; import com.facebook.react.module.annotations.ReactModule; /** @@ -23,10 +25,21 @@ import com.facebook.react.module.annotations.ReactModule; @ReactModule(name = "RCTSourceCode") public class SourceCodeModule extends BaseJavaModule { - private final String mSourceUrl; + private final ReactContext mReactContext; + private @Nullable String mSourceUrl; - public SourceCodeModule(String sourceUrl) { - mSourceUrl = sourceUrl; + public SourceCodeModule(ReactContext reactContext) { + mReactContext = reactContext; + } + + @Override + public void initialize() { + super.initialize(); + + mSourceUrl = + Assertions.assertNotNull( + mReactContext.getCatalystInstance().getSourceURL(), + "No source URL loaded, have you initialised the instance?"); } @Override