Skip to content
This repository was archived by the owner on May 20, 2025. It is now read-only.

Prevent removing ReactNativeDevBundle in LiveReload #1332

Merged
merged 4 commits into from
Jul 9, 2018
Merged

Prevent removing ReactNativeDevBundle in LiveReload #1332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f06f73b
Prevent removing ReactNativeDevBundle in LiveReload
Jul 4, 2018
bdd3f57
Fix spaces.
Jul 5, 2018
b6be387
Added comments.
Jul 5, 2018
18442f1
Fixed typo.
Jul 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions android/app/src/main/java/com/microsoft/codepush/react/CodePush.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
import com.facebook.react.bridge.JavaScriptModule;
import com.facebook.react.bridge.NativeModule;
import com.facebook.react.bridge.ReactApplicationContext;
import com.facebook.react.devsupport.DevInternalSettings;
import com.facebook.react.devsupport.interfaces.DevSupportManager;
import com.facebook.react.uimanager.ViewManager;

import org.json.JSONException;
Expand Down Expand Up @@ -76,7 +78,7 @@ public CodePush(String deploymentKey, Context context, boolean isDebugMode) {

mCurrentInstance = this;

clearDebugCacheIfNeeded();
clearDebugCacheIfNeeded(null);
initializeUpdateAfterRestart();
}

Expand Down Expand Up @@ -119,8 +121,20 @@ private String getPublicKeyByResourceDescriptor(int publicKeyResourceDescriptor)
return publicKey;
}

public void clearDebugCacheIfNeeded() {
if (mIsDebugMode && mSettingsManager.isPendingUpdate(null)) {
public void clearDebugCacheIfNeeded(ReactInstanceManager instanceManager) {
boolean isLiveReloadEnabled = false;

// Use instanceManager for checking if we use LiveRelaod mode. In this case we should not remove ReactNativeDevBundle.js file
// because we get error with trying to get this after reloading. Issue: https://github.com/Microsoft/react-native-code-push/issues/1272
if (instanceManager != null) {
DevSupportManager devSupportManager = instanceManager.getDevSupportManager();
if (devSupportManager != null) {
DevInternalSettings devInternalSettings = (DevInternalSettings)devSupportManager.getDevSettings();
isLiveReloadEnabled = devInternalSettings.isReloadOnJSChangeEnabled();
}
}

if (mIsDebugMode && mSettingsManager.isPendingUpdate(null) && !isLiveReloadEnabled) {
// This needs to be kept in sync with https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManager.java#L78
File cachedDevBundle = new File(mContext.getFilesDir(), "ReactNativeDevBundle.js");
if (cachedDevBundle.exists()) {
Expand Down
8 changes: 7 additions & 1 deletion android/app/src/main/java/com/microsoft/codepush/react/CodePushNativeModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,13 @@ private void setJSBundle(ReactInstanceManager instanceManager, String latestJSBu

private void loadBundle() {
clearLifecycleEventListener();
mCodePush.clearDebugCacheIfNeeded();
try {
mCodePush.clearDebugCacheIfNeeded(resolveInstanceManager());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reuse final ReactInstanceManager instanceManager = resolveInstanceManager(); variable from line 125 (from below) in clearDebugCacheIfNeeded to avoid unnecessary using reflection twice and additional try-catch block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we can, but we have some cases when resolveInstanceManager() has errors. In this case we should use clearDebugCacheIfNeeded with null argument. I think that adding this logic into next try-catch block is unnecessary as it make it more difficult for understanding.
Could you please share your thoughts about it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, I believe current solution fits well. The only thing that I'd probably add is a comment with e.g. reference to the issue or some explanation of the intent because it could be not evident why do we need this logic at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good suggestion. Thanks. I added comments.

} catch(Exception e) {
// If we got error in out reflection we should clear debug cache anyway.
mCodePush.clearDebugCacheIfNeeded(null);
}

try {
// #1) Get the ReactInstanceManager instance, which is what includes the
// logic to reload the current React context.
Expand Down