Make HMR server send full list modules that changed

Summary:
public

Before this diff we were only accepting the module that was modified but the user. This works fine as long as the user doesn't modify the dependencies a module has but once he starts doing so the HMR runtime may fail when updating modules' code because they might might a few dependencies. For instance, if the user changes the `src` a `Image` has to reference an image (using the new asset system) that wasn't on the original bundle the user will get a red box. This diff addresses this by diffing the modules the app currently has with the new ones it should have and including all of them on the HMR update. Note this diffing is only done when the we realize the module that was modified changed it's dependencies so there's no additional overhead on this change.

Reviewed By: vjeux

Differential Revision: D2796325

fb-gh-sync-id: cac95f2e995310634c221bbbb09d9f3e7bc03e8d

local-cli/server/util/attachHMRServer.js

@@ -35,27 +35,47 @@ function attachHMRServer({httpServer, path, packagerServer}) {
       // for each dependency builds the object:
       // `{path: '/a/b/c.js', deps: ['modA', 'modB', ...]}`
       return Promise.all(Object.values(response.dependencies).map(dep => {
-        if (dep.isAsset() || dep.isAsset_DEPRECATED() || dep.isJSON()) {
+        return dep.getName().then(depName => {
-          return Promise.resolve({path: dep.path, deps: []});
+          if (dep.isAsset() || dep.isAsset_DEPRECATED() || dep.isJSON()) {
-        }
+            return Promise.resolve({path: dep.path, deps: []});
-        return packagerServer.getShallowDependencies(dep.path)
+          }
-          .then(deps => {
+          return packagerServer.getShallowDependencies(dep.path)
-            return {
+            .then(deps => {
-              path: dep.path,
+              return {
-              deps,
+                path: dep.path,
-            };
+                name: depName,
-          });
+                deps,
+              };
+            });
+        });
       }))
       .then(deps => {
-        // list with all the dependencies the bundle entry has
+        // list with all the dependencies' filenames the bundle entry has
         const dependenciesCache = response.dependencies.map(dep => dep.path);
 
+        // map from module name to path
+        const moduleToFilenameCache = Object.create(null);
+        deps.forEach(dep => moduleToFilenameCache[dep.name] = dep.path);
+
         // map that indicates the shallow dependency each file included on the
         // bundle has
-        const shallowDependencies = {};
+        const shallowDependencies = Object.create(null);
         deps.forEach(dep => shallowDependencies[dep.path] = dep.deps);
 
-        return {dependenciesCache, shallowDependencies};
+        // map from module name to the modules' dependencies the bundle entry
+        // has
+        const dependenciesModulesCache = Object.create(null);
+        return Promise.all(response.dependencies.map(dep => {
+          return dep.getName().then(depName => {
+            dependenciesModulesCache[depName] = dep;
+          });
+        })).then(() => {
+          return {
+            dependenciesCache,
+            dependenciesModulesCache,
+            shallowDependencies,
+          };
+        });
       });
     });
   }
@@ -72,18 +92,23 @@ function attachHMRServer({httpServer, path, packagerServer}) {
     const params = querystring.parse(url.parse(ws.upgradeReq.url).query);
 
     getDependencies(params.platform, params.bundleEntry)
-      .then(({dependenciesCache, shallowDependencies}) => {
+      .then(({
+        dependenciesCache,
+        dependenciesModulesCache,
+        shallowDependencies,
+      }) => {
         client = {
           ws,
           platform: params.platform,
           bundleEntry: params.bundleEntry,
           dependenciesCache,
+          dependenciesModulesCache,
           shallowDependencies,
         };
 
         packagerServer.setHMRFileChangeListener(filename => {
           if (!client) {
-            return Promise.resolve();
+            return;
           }
 
           return packagerServer.getShallowDependencies(filename)
@@ -91,28 +116,49 @@ function attachHMRServer({httpServer, path, packagerServer}) {
               // if the file dependencies have change we need to invalidate the
               // dependencies caches because the list of files we need to send
               // to the client may have changed
-              if (arrayEquals(deps, client.shallowDependencies[filename])) {
+              const oldDependencies = client.shallowDependencies[filename];
-                return Promise.resolve();
+              if (arrayEquals(deps, oldDependencies)) {
+                return [packagerServer.getModuleForPath(filename)];
               }
+
+              // if there're new dependencies compare the full list of
+              // dependencies we used to have with the one we now have
               return getDependencies(client.platform, client.bundleEntry)
-                .then(({dependenciesCache, shallowDependencies}) => {
+                .then(({
+                  dependenciesCache,
+                  dependenciesModulesCache,
+                  shallowDependencies,
+                }) => {
+                  // build list of modules for which we'll send HMR updates
+                  const modulesToUpdate = [];
+                  Object.keys(dependenciesModulesCache).forEach(module => {
+                    if (!client.dependenciesModulesCache[module]) {
+                      modulesToUpdate.push(dependenciesModulesCache[module]);
+                    }
+                  });
+
                   // invalidate caches
                   client.dependenciesCache = dependenciesCache;
+                  client.dependenciesModulesCache = dependenciesModulesCache;
                   client.shallowDependencies = shallowDependencies;
+
+                  return modulesToUpdate;
                 });
             })
-            .then(() => {
+            .then(modulesToUpdate => {
               // make sure the file was modified is part of the bundle
               if (!client.shallowDependencies[filename]) {
                 return;
               }
 
-              return packagerServer.buildBundleForHMR({
+              return packagerServer.buildBundleForHMR(modulesToUpdate);
-                platform: client.platform,
+            })
-                entryFile: filename,
+            .then(bundle => {
-              })
+              if (bundle) {
-              .then(bundle => client.ws.send(bundle));
+                client.ws.send(bundle);
-            });
+              }
+            })
+            .done();
         });
 
         client.ws.on('error', e => {

packager/react-packager/src/Bundler/index.js

@@ -284,31 +284,29 @@ class Bundler {
     });
   }
 
-  bundleForHMR({
+  bundleForHMR(modules) {
-    entryFile,
+    return Promise.all(
-    platform,
+      modules.map(module => {
-  }) {
+        return Promise.all([
-    return this.getDependencies(entryFile, /*isDev*/true, platform).then(response => {
+          module.getName(),
-      const module = response.dependencies.filter(module => module.path === entryFile)[0];
+          this._transformer.loadFileAndTransform(
-
+            module.path,
-      return Promise.all([
+            // TODO(martinb): pass non null main (t9527509)
-        module.getName(),
+            this._getTransformOptions({main: null}, {hot: true}),
-        this._transformer.loadFileAndTransform(
+          ),
-          path.resolve(entryFile),
+        ]).then(([moduleName, transformedSource]) => {
-          // TODO(martinb): pass non null main (t9527509)
+          return (`
-          this._getTransformOptions({main: null}, {hot: true}),
+            __accept(
-        ),
+              '${moduleName}',
-      ]).then(([moduleName, transformedSource]) => {
+              function(global, require, module, exports) {
-        return (`
+                ${transformedSource.code}
-          __accept(
+              }
-            '${moduleName}',
+            );
-            function(global, require, module, exports) {
+          `);
-              ${transformedSource.code}
+        });
-            }
+      })
-          );
+    )
-        `);
+    .then(code => code.join('\n'));
-      });
-    });
   }
 
   invalidateFile(filePath) {
@@ -319,6 +317,10 @@ class Bundler {
     return this._resolver.getShallowDependencies(entryFile);
   }
 
+  getModuleForPath(entryFile) {
+    return this._resolver.getModuleForPath(entryFile);
+  }
+
   getDependencies(main, isDev, platform) {
     return this._resolver.getDependencies(main, { dev: isDev, platform });
   }

packager/react-packager/src/DependencyResolver/DependencyGraph/index.js

@@ -143,6 +143,13 @@ class DependencyGraph {
     return this._moduleCache.getModule(entryPath).getDependencies();
   }
 
+  /**
+   * Returns the module object for the given path.
+   */
+  getModuleForPath(entryFile) {
+    return this._moduleCache.getModule(entryFile);
+  }
+
   getDependencies(entryPath, platform) {
     return this.load().then(() => {
       platform = this._getRequestPlatform(entryPath, platform);

packager/react-packager/src/Resolver/index.js

@@ -104,6 +104,10 @@ class Resolver {
     return this._depGraph.getShallowDependencies(entryFile);
   }
 
+  getModuleForPath(entryFile) {
+    return this._depGraph.getModuleForPath(entryFile);
+  }
+
   getDependencies(main, options) {
     const opts = getDependenciesValidateOpts(options);
 

packager/react-packager/src/Resolver/polyfills/require.js

@@ -99,12 +99,8 @@
       var mod = modules[id];
 
       if (!mod) {
-        console.warn(
+        define(id, factory);
-          'Cannot accept unknown module `' + id + '`. Make sure you\'re not ' +
+        return; // new modules don't need to be accepted
-          'trying to modify something else other than a module ' +
-          '(i.e.: a polyfill).'
-        );
-        return;
       }
 
       if (!mod.module.hot) {

packager/react-packager/src/Server/index.js

@@ -120,17 +120,6 @@ const bundleOpts = declareOpts({
   },
 });
 
-const hmrBundleOpts = declareOpts({
-  entryFile: {
-    type: 'string',
-    required: true,
-  },
-  platform: {
-    type: 'string',
-    required: true,
-  },
-});
-
 const dependencyOpts = declareOpts({
   platform: {
     type: 'string',
@@ -199,13 +188,10 @@ class Server {
       // updates. Instead, send the HMR updates right away and once that
       // finishes, invoke any other file change listener.
       if (this._hmrFileChangeListener) {
-        this._hmrFileChangeListener(filePath).then(() => {
+        this._hmrFileChangeListener(filePath);
-          this._fileChangeListeners.forEach(listener => listener(filePath));
-        }).done();
         return;
       }
 
-      this._fileChangeListeners.forEach(listener => listener(filePath));
       this._rebuildBundles(filePath);
       this._informChangeWatchers();
     }, 50);
@@ -218,10 +204,6 @@ class Server {
     ]);
   }
 
-  addFileChangeListener(listener) {
-    this._fileChangeListeners.push(listener);
-  }
-
   setHMRFileChangeListener(listener) {
     this._hmrFileChangeListener = listener;
   }
@@ -253,21 +235,18 @@ class Server {
     return this.buildBundle(options);
   }
 
-  buildBundleForHMR(options) {
+  buildBundleForHMR(modules) {
-    return Promise.resolve().then(() => {
+    return this._bundler.bundleForHMR(modules);
-      if (!options.platform) {
-        options.platform = getPlatformExtension(options.entryFile);
-      }
-
-      const opts = hmrBundleOpts(options);
-      return this._bundler.bundleForHMR(opts);
-    });
   }
 
   getShallowDependencies(entryFile) {
     return this._bundler.getShallowDependencies(entryFile);
   }
 
+  getModuleForPath(entryFile) {
+    return this._bundler.getModuleForPath(entryFile);
+  }
+
   getDependencies(options) {
     return Promise.resolve().then(() => {
       if (!options.platform) {