From f2bdb7978260720f48b4b082e17b4474721a3ec1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Bigio?= Date: Mon, 4 Jan 2016 13:01:28 -0800 Subject: [PATCH] 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 | 96 ++++++++++++++----- packager/react-packager/src/Bundler/index.js | 52 +++++----- .../DependencyGraph/index.js | 7 ++ packager/react-packager/src/Resolver/index.js | 4 + .../src/Resolver/polyfills/require.js | 8 +- packager/react-packager/src/Server/index.js | 35 ++----- 6 files changed, 118 insertions(+), 84 deletions(-) diff --git a/local-cli/server/util/attachHMRServer.js b/local-cli/server/util/attachHMRServer.js index e039edef5..2b1ff4334 100644 --- a/local-cli/server/util/attachHMRServer.js +++ b/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 Promise.resolve({path: dep.path, deps: []}); - } - return packagerServer.getShallowDependencies(dep.path) - .then(deps => { - return { - path: dep.path, - deps, - }; - }); + return dep.getName().then(depName => { + if (dep.isAsset() || dep.isAsset_DEPRECATED() || dep.isJSON()) { + return Promise.resolve({path: dep.path, deps: []}); + } + return packagerServer.getShallowDependencies(dep.path) + .then(deps => { + return { + 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])) { - return Promise.resolve(); + const oldDependencies = client.shallowDependencies[filename]; + 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({ - platform: client.platform, - entryFile: filename, - }) - .then(bundle => client.ws.send(bundle)); - }); + return packagerServer.buildBundleForHMR(modulesToUpdate); + }) + .then(bundle => { + if (bundle) { + client.ws.send(bundle); + } + }) + .done(); }); client.ws.on('error', e => { diff --git a/packager/react-packager/src/Bundler/index.js b/packager/react-packager/src/Bundler/index.js index 7fdbe911d..bbb4d3bb4 100644 --- a/packager/react-packager/src/Bundler/index.js +++ b/packager/react-packager/src/Bundler/index.js @@ -284,31 +284,29 @@ class Bundler { }); } - bundleForHMR({ - entryFile, - platform, - }) { - return this.getDependencies(entryFile, /*isDev*/true, platform).then(response => { - const module = response.dependencies.filter(module => module.path === entryFile)[0]; - - return Promise.all([ - module.getName(), - this._transformer.loadFileAndTransform( - path.resolve(entryFile), - // TODO(martinb): pass non null main (t9527509) - this._getTransformOptions({main: null}, {hot: true}), - ), - ]).then(([moduleName, transformedSource]) => { - return (` - __accept( - '${moduleName}', - function(global, require, module, exports) { - ${transformedSource.code} - } - ); - `); - }); - }); + bundleForHMR(modules) { + return Promise.all( + modules.map(module => { + return Promise.all([ + module.getName(), + this._transformer.loadFileAndTransform( + module.path, + // TODO(martinb): pass non null main (t9527509) + this._getTransformOptions({main: null}, {hot: true}), + ), + ]).then(([moduleName, transformedSource]) => { + return (` + __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 }); } diff --git a/packager/react-packager/src/DependencyResolver/DependencyGraph/index.js b/packager/react-packager/src/DependencyResolver/DependencyGraph/index.js index c4e983ad9..ec0d964e0 100644 --- a/packager/react-packager/src/DependencyResolver/DependencyGraph/index.js +++ b/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); diff --git a/packager/react-packager/src/Resolver/index.js b/packager/react-packager/src/Resolver/index.js index ecc9eeebe..efa61d576 100644 --- a/packager/react-packager/src/Resolver/index.js +++ b/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); diff --git a/packager/react-packager/src/Resolver/polyfills/require.js b/packager/react-packager/src/Resolver/polyfills/require.js index afcda4ca1..36c2b6b90 100644 --- a/packager/react-packager/src/Resolver/polyfills/require.js +++ b/packager/react-packager/src/Resolver/polyfills/require.js @@ -99,12 +99,8 @@ var mod = modules[id]; if (!mod) { - console.warn( - 'Cannot accept unknown module `' + id + '`. Make sure you\'re not ' + - 'trying to modify something else other than a module ' + - '(i.e.: a polyfill).' - ); - return; + define(id, factory); + return; // new modules don't need to be accepted } if (!mod.module.hot) { diff --git a/packager/react-packager/src/Server/index.js b/packager/react-packager/src/Server/index.js index 3e18a23cd..858bc0e4f 100644 --- a/packager/react-packager/src/Server/index.js +++ b/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._fileChangeListeners.forEach(listener => listener(filePath)); - }).done(); + this._hmrFileChangeListener(filePath); 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) { - return Promise.resolve().then(() => { - if (!options.platform) { - options.platform = getPlatformExtension(options.entryFile); - } - - const opts = hmrBundleOpts(options); - return this._bundler.bundleForHMR(opts); - }); + buildBundleForHMR(modules) { + return this._bundler.bundleForHMR(modules); } getShallowDependencies(entryFile) { return this._bundler.getShallowDependencies(entryFile); } + getModuleForPath(entryFile) { + return this._bundler.getModuleForPath(entryFile); + } + getDependencies(options) { return Promise.resolve().then(() => { if (!options.platform) {