Skip to content

Commit

Permalink
Make HMR faster
Browse files Browse the repository at this point in the history
Summary:
public

At the moment, when the user changes a file we end up pulling the dependencies of the entry point to build the bundle. This could take a long time if the bundle is big. To avoid it, lets introduce a new parameter to `getDependencies` to be able to avoid processing the modules recursively and reuse the resolution responseto build the bundle.

Reviewed By: davidaurelio

Differential Revision: D2862850

fb-gh-sync-id: b8ae2b811a8ae9aec5612f9655d1c762671ce730
  • Loading branch information
martinbigio authored and facebook-github-bot-9 committed Jan 29, 2016
1 parent c8a0a3e commit 65b8ff1
Show file tree
Hide file tree
Showing 8 changed files with 112 additions and 36 deletions.
30 changes: 26 additions & 4 deletions local-cli/server/util/attachHMRServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ function attachHMRServer({httpServer, path, packagerServer}) {
dependenciesCache,
dependenciesModulesCache,
shallowDependencies,
resolutionResponse: response,
};
});
});
Expand Down Expand Up @@ -123,7 +124,23 @@ function attachHMRServer({httpServer, path, packagerServer}) {
// to the client may have changed
const oldDependencies = client.shallowDependencies[filename];
if (arrayEquals(deps, oldDependencies)) {
return [packagerServer.getModuleForPath(filename)];
// Need to create a resolution response to pass to the bundler
// to process requires after transform. By providing a
// specific response we can compute a non recursive one which
// is the least we need and improve performance.
return packagerServer.getDependencies({
platform: client.platform,
dev: true,
entryFile: filename,
recursive: true,
}).then(response => {
const module = packagerServer.getModuleForPath(filename);

return {
modulesToUpdate: [module],
resolutionResponse: response,
};
});
}

// if there're new dependencies compare the full list of
Expand All @@ -133,9 +150,10 @@ function attachHMRServer({httpServer, path, packagerServer}) {
dependenciesCache,
dependenciesModulesCache,
shallowDependencies,
resolutionResponse,
}) => {
if (!client) {
return [];
return {};
}

// build list of modules for which we'll send HMR updates
Expand All @@ -151,10 +169,13 @@ function attachHMRServer({httpServer, path, packagerServer}) {
client.dependenciesModulesCache = dependenciesModulesCache;
client.shallowDependencies = shallowDependencies;

return modulesToUpdate;
return {
modulesToUpdate,
resolutionResponse,
};
});
})
.then(modulesToUpdate => {
.then(({modulesToUpdate, resolutionResponse}) => {
if (!client) {
return;
}
Expand All @@ -168,6 +189,7 @@ function attachHMRServer({httpServer, path, packagerServer}) {
entryFile: client.bundleEntry,
platform: client.platform,
modules: modulesToUpdate,
resolutionResponse,
})
})
.then(bundle => {
Expand Down
11 changes: 6 additions & 5 deletions packager/react-packager/src/Bundler/__tests__/Bundler-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,11 +206,12 @@ describe('Bundler', function() {
});

pit('gets the list of dependencies from the resolver', function() {
return bundler.getDependencies('/root/foo.js', true)
.then(
() => expect(getDependencies)
.toBeCalledWith('/root/foo.js', { dev: true })
);
return bundler.getDependencies('/root/foo.js', true).then(() =>
expect(getDependencies).toBeCalledWith(
'/root/foo.js',
{ dev: true, recursive: true },
)
);
});

describe('getOrderedDependencyPaths', () => {
Expand Down
63 changes: 49 additions & 14 deletions packager/react-packager/src/Bundler/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,14 +235,48 @@ class Bundler {
unbundle: isUnbundle,
hot: hot,
entryModuleOnly,
resolutionResponse,
}) {
const findEventId = Activity.startEvent('find dependencies');
let transformEventId;
const moduleSystemDeps = includeSystemDependencies
? this._resolver.getModuleSystemDependencies(
{ dev: isDev, platform, isUnbundle }
)
: [];

const findModules = () => {
const findEventId = Activity.startEvent('find dependencies');
return this.getDependencies(entryFile, isDev, platform).then(response => {
Activity.endEvent(findEventId);
bundle.setMainModuleId(response.mainModuleId);
bundle.setMainModuleName(response.mainModuleId);
if (!entryModuleOnly && bundle.setNumPrependedModules) {
bundle.setNumPrependedModules(
response.numPrependedDependencies + moduleSystemDeps.length
);
}

return {
response,
modulesToProcess: response.dependencies,
};
});
};

const useProvidedModules = () => {
const moduleId = this._resolver.getModuleForPath(entryFile);
bundle.setMainModuleId(moduleId);
bundle.setMainModuleName(moduleId);
return Promise.resolve({
response: resolutionResponse,
modulesToProcess: modules
});
};

return (
modules ? useProvidedModules() : findModules()
).then(({response, modulesToProcess}) => {

return this.getDependencies(entryFile, isDev, platform).then((response) => {
Activity.endEvent(findEventId);
bundle.setMainModuleId(response.mainModuleId);
bundle.setMainModuleName(response.mainModuleId);
transformEventId = Activity.startEvent('transform');

let dependencies;
Expand All @@ -259,10 +293,6 @@ class Bundler {

const modulesToProcess = modules || response.dependencies;
dependencies = moduleSystemDeps.concat(modulesToProcess);

bundle.setNumPrependedModules && bundle.setNumPrependedModules(
response.numPrependedDependencies + moduleSystemDeps.length
);
}

let bar;
Expand All @@ -280,7 +310,6 @@ class Bundler {
module => {
return this._transformModule(
bundle,
response,
module,
platform,
isDev,
Expand Down Expand Up @@ -347,7 +376,6 @@ class Bundler {
response.dependencies.map(
module => this._transformModule(
bundle,
response,
module,
platform,
isDev,
Expand Down Expand Up @@ -418,8 +446,15 @@ class Bundler {
return this._resolver.getModuleForPath(entryFile);
}

getDependencies(main, isDev, platform) {
return this._resolver.getDependencies(main, { dev: isDev, platform });
getDependencies(main, isDev, platform, recursive = true) {
return this._resolver.getDependencies(
main,
{
dev: isDev,
platform,
recursive,
},
);
}

getOrderedDependencyPaths({ entryFile, dev, platform }) {
Expand Down Expand Up @@ -454,7 +489,7 @@ class Bundler {
);
}

_transformModule(bundle, response, module, platform = null, dev = true, hot = false) {
_transformModule(bundle, module, platform = null, dev = true, hot = false) {
if (module.isAsset_DEPRECATED()) {
return this._generateAssetModule_DEPRECATED(bundle, module);
} else if (module.isAsset()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ class ResolutionRequest {
);
}

getOrderedDependencies(response, mocksPattern) {
getOrderedDependencies(response, mocksPattern, recursive = true) {
return this._getAllMocks(mocksPattern).then(allMocks => {
const entry = this._moduleCache.getModule(this._entryPath);
const mocks = Object.create(null);
Expand Down Expand Up @@ -163,7 +163,9 @@ class ResolutionRequest {
p = p.then(() => {
if (!visited[modDep.hash()]) {
visited[modDep.hash()] = true;
return collect(modDep);
if (recursive) {
return collect(modDep);
}
}
return null;
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ class DependencyGraph {
return this._moduleCache.getModule(entryFile);
}

getDependencies(entryPath, platform) {
getDependencies(entryPath, platform, recursive = true) {
return this.load().then(() => {
platform = this._getRequestPlatform(entryPath, platform);
const absPath = this._getAbsolutePath(entryPath);
Expand All @@ -177,7 +177,11 @@ class DependencyGraph {
const response = new ResolutionResponse();

return Promise.all([
req.getOrderedDependencies(response, this._opts.mocksPattern),
req.getOrderedDependencies(
response,
this._opts.mocksPattern,
recursive,
),
req.getAsyncDependencies(response),
]).then(() => response);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,8 @@ describe('Resolver', function() {
return depResolver.getDependencies('/root/index.js', { dev: true })
.then(function(result) {
expect(result.mainModuleId).toEqual('index');
expect(DependencyGraph.mock.instances[0].getDependencies).toBeCalledWith('/root/index.js', undefined);
expect(DependencyGraph.mock.instances[0].getDependencies)
.toBeCalledWith('/root/index.js', undefined, true);
expect(result.dependencies[0]).toBe(Polyfill.mock.instances[0]);
expect(result.dependencies[result.dependencies.length - 1])
.toBe(module);
Expand Down
22 changes: 14 additions & 8 deletions packager/react-packager/src/Resolver/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ const getDependenciesValidateOpts = declareOpts({
type: 'boolean',
default: false
},
recursive: {
type: 'boolean',
default: true,
},
});

class Resolver {
Expand Down Expand Up @@ -116,15 +120,17 @@ class Resolver {
getDependencies(main, options) {
const opts = getDependenciesValidateOpts(options);

return this._depGraph.getDependencies(main, opts.platform).then(
resolutionResponse => {
this._getPolyfillDependencies().reverse().forEach(
polyfill => resolutionResponse.prependDependency(polyfill)
);
return this._depGraph.getDependencies(
main,
opts.platform,
opts.recursive,
).then(resolutionResponse => {
this._getPolyfillDependencies().reverse().forEach(
polyfill => resolutionResponse.prependDependency(polyfill)
);

return resolutionResponse.finalize();
}
);
return resolutionResponse.finalize();
});
}

getModuleSystemDependencies(options) {
Expand Down
5 changes: 5 additions & 0 deletions packager/react-packager/src/Server/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ const dependencyOpts = declareOpts({
type: 'string',
required: true,
},
recursive: {
type: 'boolean',
default: true,
},
});

class Server {
Expand Down Expand Up @@ -269,6 +273,7 @@ class Server {
opts.entryFile,
opts.dev,
opts.platform,
opts.recursive,
);
});
}
Expand Down

0 comments on commit 65b8ff1

Please sign in to comment.