From e81adb99f318582f5cd877e9c20614c1cec577ed Mon Sep 17 00:00:00 2001 From: Alexandre Kirszenberg Date: Wed, 28 Nov 2018 02:48:51 -0800 Subject: [PATCH] DeltaClient: split DeltaBundle's modules into added and modified Summary: The reasoning behind this change is that right now, having both added and modified modules inside of a single `modules` field doesn't allow for basic operations like combining two deltas. For instance, say I have three different bundle revisions: A, B and C. Module 42 was added in B, and then removed in C. A->B = `{modules: [42, "..."], deleted: []}` B->C = `{modules: [], deleted: [42]}` A->C = `{modules: [], deleted: []}` However, were we to compute A->C as the combination of A->B and B->C, it would result in `{modules: [], deleted: [42]}` because we have no way of knowing that module 42 was only just added in B. This means that the `deleted` field of delta X->Y might eventually contain module ids that were never present in revision X, because they were added and then removed between revisions X and Y. The last time I changed the delta format, we had a few bug reports pop out from people who had desync issues between their version of React Native and their version of Metro. As such, I've tried to make this change backwards compatible in at least one direction (new RN, old Metro). However, this will still break if someone is using a newer version of Metro and an older version of RN. I created T37123645 to follow up on this. Reviewed By: rafeca, fromcelticpark Differential Revision: D13156514 fbshipit-source-id: 4a4ee3b6cc0cdff5dca7368a46d7bf663769e281 --- .../react/devsupport/BundleDeltaClient.java | 4 +++ ReactCommon/cxxreact/JSDeltaBundleClient.cpp | 36 +++++++++++++++---- ReactCommon/cxxreact/JSDeltaBundleClient.h | 2 ++ .../server/util/debugger-ui/DeltaPatcher.js | 11 ++++-- 4 files changed, 43 insertions(+), 10 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/devsupport/BundleDeltaClient.java b/ReactAndroid/src/main/java/com/facebook/react/devsupport/BundleDeltaClient.java index bdc6d084407..e035c976a52 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/devsupport/BundleDeltaClient.java +++ b/ReactAndroid/src/main/java/com/facebook/react/devsupport/BundleDeltaClient.java @@ -105,6 +105,10 @@ public abstract class BundleDeltaClient { mPostCode = jsonReader.nextString().getBytes(); } else if (name.equals("modules")) { numChangedModules += setModules(jsonReader, mModules); + } else if (name.equals("added")) { + numChangedModules += setModules(jsonReader, mModules); + } else if (name.equals("modified")) { + numChangedModules += setModules(jsonReader, mModules); } else if (name.equals("deleted")) { numChangedModules += removeModules(jsonReader, mModules); } else { diff --git a/ReactCommon/cxxreact/JSDeltaBundleClient.cpp b/ReactCommon/cxxreact/JSDeltaBundleClient.cpp index 86edf47c800..2f66e861eb4 100644 --- a/ReactCommon/cxxreact/JSDeltaBundleClient.cpp +++ b/ReactCommon/cxxreact/JSDeltaBundleClient.cpp @@ -26,6 +26,14 @@ namespace { } } // namespace +void JSDeltaBundleClient::patchModules(const folly::dynamic *modules) { + for (const folly::dynamic pair : *modules) { + auto id = pair[0].getInt(); + auto module = pair[1]; + modules_.emplace(id, module.getString()); + } +} + void JSDeltaBundleClient::patch(const folly::dynamic& delta) { auto const base = delta.get_ptr("base"); @@ -36,6 +44,11 @@ void JSDeltaBundleClient::patch(const folly::dynamic& delta) { auto const post = delta.get_ptr("post"); startupCode_ = startupCode(pre, post); + + const folly::dynamic *modules = delta.get_ptr("modules"); + if (modules != nullptr) { + patchModules(modules); + } } else { const folly::dynamic *deleted = delta.get_ptr("deleted"); if (deleted != nullptr) { @@ -43,16 +56,25 @@ void JSDeltaBundleClient::patch(const folly::dynamic& delta) { modules_.erase(id.getInt()); } } - } - const folly::dynamic *modules = delta.get_ptr("modules"); - if (modules != nullptr) { - for (const folly::dynamic pair : *modules) { - auto id = pair[0].getInt(); - auto module = pair[1]; - modules_.emplace(id, module.getString()); + // TODO T37123645 This is deprecated but necessary in order to support older + // versions of the Metro server. + const folly::dynamic *modules = delta.get_ptr("modules"); + if (modules != nullptr) { + patchModules(modules); + } + + const folly::dynamic *added = delta.get_ptr("added"); + if (added != nullptr) { + patchModules(added); + } + + const folly::dynamic *modified = delta.get_ptr("modified"); + if (modified != nullptr) { + patchModules(modified); } } + } JSModulesUnbundle::Module JSDeltaBundleClient::getModule(uint32_t moduleId) const { diff --git a/ReactCommon/cxxreact/JSDeltaBundleClient.h b/ReactCommon/cxxreact/JSDeltaBundleClient.h index 823beab076f..5ee2aba9ca7 100644 --- a/ReactCommon/cxxreact/JSDeltaBundleClient.h +++ b/ReactCommon/cxxreact/JSDeltaBundleClient.h @@ -27,6 +27,8 @@ public: private: std::unordered_map modules_; std::string startupCode_; + + void patchModules(const folly::dynamic *delta); }; class JSDeltaBundleClientRAMBundle : public JSModulesUnbundle { diff --git a/local-cli/server/util/debugger-ui/DeltaPatcher.js b/local-cli/server/util/debugger-ui/DeltaPatcher.js index 7d7734f08e3..ed950295f6c 100644 --- a/local-cli/server/util/debugger-ui/DeltaPatcher.js +++ b/local-cli/server/util/debugger-ui/DeltaPatcher.js @@ -72,12 +72,17 @@ modules: new Map(bundle.modules), }; } else { - this._lastNumModifiedFiles = - bundle.modules.length + bundle.deleted.length; + // TODO T37123645 The former case is deprecated, but necessary in order to + // support older versions of the Metro bundler. + const modules = bundle.modules + ? bundle.modules + : bundle.added.concat(bundle.modified); + + this._lastNumModifiedFiles = modules.length + bundle.deleted.length; this._lastBundle.revisionId = bundle.revisionId; - for (const [key, value] of bundle.modules) { + for (const [key, value] of modules) { this._lastBundle.modules.set(key, value); }