Skip to content

Commit

Permalink
Kill fastfs
Browse files Browse the repository at this point in the history
Summary:
This kills fastfs in favor of Jest's hasteFS. It gets rid of a ton of code, including the mocking code in ResolutionRequest which we don't need any more. Next step after this is to rewrite HasteMap, ModuleCache, Module/Package. We are getting closer to a nicer and faster world! :)

Here is what I did:
* Use Jest's HasteFS instead of fastfs. A fresh instance is received every time something changes on the FS.
* HasteFS is not shared with everything any more. Only one reference is kept in DependencyGraph and there are a few smaller functions that are passed around (getClosestPackage and dirExists). Note: `dirExists` now does fs access instead of an offline check. This sucks but stat calls aren't slow and aren't going to be a bottleneck in ResolutionRequest, I promise! When it is time to tackle a ResolutionRequest rewrite with jest-resolve, this will go away. "It gets worse before it gets better" :) The ModuleGraph equivalent does *not* do fs access and retains the previous way of doing things because we shouldn't do online fs access there.
* Add flow annotations to ResolutionRequest. This required a few tiny hacks for now because of ModuleGraph's duck typing. I'll get rid of this soon.
* Updated ModuleGraph to work with the new code, also created a mock HasteFS instance there.
* I fixed a few tiny mock issues for `fs` to make the tests work; I had to add one tiny little internal update to `dgraph._hasteFS._files` because the file watching in the tests isn't real. It is instrumented through some function calls, therefore the hasteFS instance doesn't get automatically updated. One way to solve this is to add `JestHasteMap.emit('change', …)` for testing but I didn't want to cut a Jest release just for that. #movefast

(Note: I will likely land this in 1.5 weeks from now after my vacation and I have yet to fully test all the product flows. Please give me feedback so I can make sure this is solid!)

Reviewed By: davidaurelio

Differential Revision: D4204082

fbshipit-source-id: d6dc9fcb77ac224df4554a59f0fce241c01b0512
  • Loading branch information
cpojer authored and Facebook Github Bot committed Nov 30, 2016
1 parent 911c05a commit 6554ad5
Show file tree
Hide file tree
Showing 21 changed files with 392 additions and 960 deletions.
25 changes: 11 additions & 14 deletions local-cli/server/util/attachHMRServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ function attachHMRServer({httpServer, path, packagerServer}) {
inverseDependenciesCache,
};

packagerServer.setHMRFileChangeListener((filename, stat) => {
packagerServer.setHMRFileChangeListener((type, filename) => {
if (!client) {
return;
}
Expand All @@ -151,14 +151,14 @@ function attachHMRServer({httpServer, path, packagerServer}) {
}

client.ws.send(JSON.stringify({type: 'update-start'}));
stat.then(() => {
return packagerServer.getShallowDependencies({
entryFile: filename,
platform: client.platform,
dev: true,
hot: true,
})
.then(deps => {
const promise = type === 'delete'
? Promise.resolve()
: packagerServer.getShallowDependencies({
entryFile: filename,
platform: client.platform,
dev: true,
hot: true,
}).then(deps => {
if (!client) {
return [];
}
Expand Down Expand Up @@ -300,11 +300,8 @@ function attachHMRServer({httpServer, path, packagerServer}) {
print(createEntry('HMR Server sending update to client'));
client.ws.send(update);
});
},
() => {
// do nothing, file was removed
},
).then(() => {

promise.then(() => {
client.ws.send(JSON.stringify({type: 'update-done'}));
});
});
Expand Down
4 changes: 0 additions & 4 deletions packager/react-packager/src/Bundler/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -502,10 +502,6 @@ class Bundler {
});
}

stat(filePath: string) {
return this._resolver.stat(filePath);
}

getModuleForPath(entryFile: string) {
return this._resolver.getModuleForPath(entryFile);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

const {dirname, join, parse} = require('path');

module.exports = class FastFS {
module.exports = class HasteFS {
directories: Set<string>;
directoryEntries: Map<string, Array<string>>;
files: Set<string>;
Expand All @@ -40,7 +40,7 @@ module.exports = class FastFS {
return this.directories.has(path);
}

fileExists(path: string) {
exists(path: string) {
return this.files.has(path);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,19 @@ const Module = require('./Module');
const Package = require('./Package');

import type {PackageData, TransformedFile} from '../types.flow';
import type {FastFS} from './node-haste.flow';

type GetFn<T> = (path: string) => Promise<T>;
type GetClosestPackageFn = (filePath: string) => ?string;

module.exports = class ModuleCache {
fastfs: FastFS;
_getClosestPackage: GetClosestPackageFn;
getPackageData: GetFn<PackageData>;
getTransformedFile: GetFn<TransformedFile>;
modules: Map<string, Module>;
packages: Map<string, Package>;

constructor(fastfs: FastFS, getTransformedFile: GetFn<TransformedFile>) {
this.fastfs = fastfs;
constructor(getClosestPackage: GetClosestPackageFn, getTransformedFile: GetFn<TransformedFile>) {
this._getClosestPackage = getClosestPackage;
this.getTransformedFile = getTransformedFile;
this.getPackageData = path => getTransformedFile(path).then(
f => f.package || Promise.reject(new Error(`"${path}" does not exist`))
Expand Down Expand Up @@ -59,7 +59,7 @@ module.exports = class ModuleCache {
}

getPackageOf(filePath: string) {
const candidate = this.fastfs.closest(filePath, 'package.json');
const candidate = this._getClosestPackage(filePath);
return candidate != null ? this.getPackage(candidate) : null;
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*
* @flow
*/
'use strict';

'use strict';

Expand Down Expand Up @@ -57,7 +58,7 @@ export type FastFS = {
type HasteMapOptions = {|
allowRelativePaths: boolean,
extensions: Extensions,
fastfs: FastFS,
files: Array<string>,
helpers: DependencyGraphHelpers,
moduleCache: ModuleCache,
platforms: Platforms,
Expand All @@ -66,26 +67,6 @@ type HasteMapOptions = {|

declare class HasteMap {
// node-haste/DependencyGraph/HasteMap.js
constructor(options: HasteMapOptions): void,
build(): Promise<Object>,
constructor(options: HasteMapOptions): void,
}
export type HasteMapT = HasteMap;

type ResolutionRequestOptions = {|
platform: Platform,
platforms: Platforms,
preferNativePlatform: true,
hasteMap: HasteMap,
helpers: DependencyGraphHelpers,
moduleCache: ModuleCache,
fastfs: FastFS,
shouldThrowOnUnresolvedErrors: () => true,
extraNodeModules: {[id: ModuleID]: Path},
|};

declare class ResolutionRequest {
// node-haste/DependencyGraph/ResolutionRequest.js
constructor(options: ResolutionRequestOptions): void,
resolveDependency(from: Module, to: ModuleID): Promise<Module>,
}
export type ResolutionRequestT = ResolutionRequest;
22 changes: 13 additions & 9 deletions packager/react-packager/src/ModuleGraph/node-haste/node-haste.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@

import type { // eslint-disable-line sort-requires
Extensions,
HasteMapT,
Path,
ResolutionRequestT,
} from './node-haste.flow';

import type {
Expand All @@ -24,11 +22,12 @@ import type {
} from '../types.flow';

const DependencyGraphHelpers = require('../../node-haste/DependencyGraph/DependencyGraphHelpers');
const FastFS = require('./FastFS');
const HasteMap: Class<HasteMapT> = require('../../node-haste/DependencyGraph/HasteMap');
const HasteFS = require('./HasteFS');
const HasteMap = require('../../node-haste/DependencyGraph/HasteMap');
const Module = require('./Module');
const ModuleCache = require('./ModuleCache');
const ResolutionRequest: Class<ResolutionRequestT> = require('../../node-haste/DependencyGraph/ResolutionRequest');
const ResolutionRequest = require('../../node-haste/DependencyGraph/ResolutionRequest');

const defaults = require('../../../../defaults');

type ResolveOptions = {|
Expand Down Expand Up @@ -57,12 +56,15 @@ exports.createResolveFn = function(options: ResolveOptions): ResolveFn {
providesModuleNodeModules: defaults.providesModuleNodeModules,
});

const fastfs = new FastFS(files);
const moduleCache = new ModuleCache(fastfs, getTransformedFile);
const hasteFS = new HasteFS(files);
const moduleCache = new ModuleCache(
filePath => hasteFS.closest(filePath, 'package.json'),
getTransformedFile,
);
const hasteMap = new HasteMap({
allowRelativePaths: true,
extensions: ['js', 'json'],
fastfs,
files,
helpers,
moduleCache,
platforms,
Expand All @@ -75,8 +77,10 @@ exports.createResolveFn = function(options: ResolveOptions): ResolveFn {
let resolutionRequest = resolutionRequests[platform];
if (!resolutionRequest) {
resolutionRequest = resolutionRequests[platform] = new ResolutionRequest({
dirExists: filePath => hasteFS.dirExists(filePath),
entryPath: '',
extraNodeModules,
fastfs,
hasteFS,
hasteMap,
helpers,
moduleCache,
Expand Down
4 changes: 0 additions & 4 deletions packager/react-packager/src/Resolver/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,6 @@ class Resolver {
return this._depGraph.getShallowDependencies(entryFile, transformOptions);
}

stat(filePath) {
return this._depGraph.getFS().stat(filePath);
}

getModuleForPath(entryFile) {
return this._depGraph.getModuleForPath(entryFile);
}
Expand Down
2 changes: 1 addition & 1 deletion packager/react-packager/src/Server/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ class Server {
if (this._hmrFileChangeListener) {
// Clear cached bundles in case user reloads
this._clearBundles();
this._hmrFileChangeListener(filePath, this._bundler.stat(filePath));
this._hmrFileChangeListener(type, filePath);
return;
} else if (type !== 'change' && filePath.indexOf(NODE_MODULES) !== -1) {
// node module resolution can be affected by added or removed files
Expand Down
9 changes: 9 additions & 0 deletions packager/react-packager/src/node-haste/AssetModule.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
'use strict';

const Module = require('./Module');

const getAssetDataFromName = require('./lib/getAssetDataFromName');

class AssetModule extends Module {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,19 @@ const PACKAGE_JSON = path.sep + 'package.json';
class HasteMap extends EventEmitter {
constructor({
extensions,
fastfs,
files,
moduleCache,
preferNativePlatform,
helpers,
platforms,
}) {
super();
this._extensions = extensions;
this._fastfs = fastfs;
this._moduleCache = moduleCache;
this._preferNativePlatform = preferNativePlatform;
this._files = files;
this._helpers = helpers;
this._moduleCache = moduleCache;
this._platforms = platforms;
this._preferNativePlatform = preferNativePlatform;

this._processHastePackage = throat(1, this._processHastePackage.bind(this));
this._processHasteModule = throat(1, this._processHasteModule.bind(this));
Expand All @@ -42,7 +42,7 @@ class HasteMap extends EventEmitter {
build() {
this._map = Object.create(null);
const promises = [];
this._fastfs.getAllFiles().forEach(filePath => {
this._files.forEach(filePath => {
if (!this._helpers.isNodeModulesDir(filePath)) {
if (this._extensions.indexOf(path.extname(filePath).substr(1)) !== -1) {
promises.push(this._processHasteModule(filePath));
Expand Down
Loading

0 comments on commit 6554ad5

Please sign in to comment.