Permalink
Browse files

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
  • Loading branch information...
amnn authored and facebook-github-bot committed Jan 13, 2017
1 parent 8e4f33e commit 89d72c99be8954976c0681b780bf32d4583a754f
@@ -128,7 +128,7 @@ public NativeModule get() {
new ModuleSpec(SourceCodeModule.class, new Provider<NativeModule>() {
@Override
public NativeModule get() {
return new SourceCodeModule(mReactInstanceManager.getSourceUrl());
return new SourceCodeModule(reactContext);
}
}));
moduleSpecList.add(
@@ -154,16 +154,6 @@ public abstract void onActivityResult(
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
@@ -127,7 +127,6 @@
private @Nullable volatile ReactContext mCurrentReactContext;
private final Context mApplicationContext;
private @Nullable DefaultHardwareBackBtnHandler mDefaultBackButtonImpl;
private String mSourceUrl;
private @Nullable Activity mCurrentActivity;
private final Collection<ReactInstanceEventListener> mReactInstanceEventListeners =
Collections.synchronizedSet(new HashSet<ReactInstanceEventListener>());
@@ -634,22 +633,6 @@ public void showDevOptionsDialog() {
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 @@ private ReactApplicationContext createReactContext(
JSBundleLoader jsBundleLoader) {
FLog.i(ReactConstants.TAG, "Creating react context.");
ReactMarker.logMarker(CREATE_REACT_CONTEXT_START);
mSourceUrl = jsBundleLoader.getSourceUrl();
List<ModuleSpec> moduleSpecs = new ArrayList<>();
Map<Class, ReactModuleInfo> reactModuleInfoMap = new HashMap<>();
JavaScriptModuleRegistry.Builder jsModulesBuilder = new JavaScriptModuleRegistry.Builder();
@@ -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 @@
@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
@@ -97,6 +97,7 @@ public PendingJSCall(
private volatile boolean mAcceptCalls = false;
private boolean mJSBundleHasLoaded;
private @Nullable String mSourceURL;
// C++ parts
private final HybridData mHybridData;
@@ -188,8 +189,9 @@ private native void initializeBridge(ReactCallback callback,
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 void runJSBundle() {
Systrace.registerListener(mTraceListener);
}
@Override
public @Nullable String getSourceURL() {
return mSourceURL;
}
private native void callJSFunction(
ExecutorToken token,
String module,
@@ -29,12 +29,8 @@ public static JSBundleLoader createAssetLoader(
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 String getSourceUrl() {
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 static JSBundleLoader createCachedBundleFromNetworkLoader(
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 static JSBundleLoader createRemoteDebuggerBundleLoader(
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);
}
@@ -24,12 +24,8 @@ public OptimizedJSBundleLoader(String path, String sourceURL, int loadFlags) {
}
@Override
public void loadScript(CatalystInstanceImpl instance) {
public String loadScript(CatalystInstanceImpl instance) {
instance.loadScriptFromOptimizedBundle(mPath, mSourceURL, mLoadFlags);
}
@Override
public String getSourceUrl() {
return mSourceURL;
}
}
@@ -14,7 +14,9 @@
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 @@
@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

0 comments on commit 89d72c9

Please sign in to comment.