From 197ebcf7bf9ed5017db2229aac53ee61ecaa4ecc Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Sun, 12 Jun 2016 20:16:11 +0200 Subject: [PATCH 01/41] Make file finding async I've been wanting to explore a watchman-powered file finder. Before I can do that, I want all file-finding to be async. This is good, because we can parallelize lookups. Instead of adding callbacks all over the place, I decided to use Promises. I believe this will make the code a little simpler to understand as it provides a nice interface for everything async. An annoying issue I ran into while switching to Promises was that I wasn't getting errors surfaced when they happened. It turns out I was bit by the auto-swallowing of errors that Promises have. I came across this article explaining things: http://jamesknelson.com/are-es6-promises-swallowing-your-errors/ In that article, a third-party solution to globally fix the issue is suggested. I decided against bringing in another dependency, and instead bite the bullet and go down the explicit (and somewhat verbose) path, using `reject` and `catch`. To make tests easier to work with, I decided to make the asyncness synchronous by adding a dependency to deasync [1]. I found that from this SO thread: http://stackoverflow.com/questions/16586640/node-js-async-to-sync In the long run, we might want to avoid having that dependency and instead make test assertions async as well. But for now it allowed me to move forward without having to rewrite the (quite massive) test suite that we have. [1]: https://github.com/abbr/deasync --- lib/Configuration.js | 3 +- lib/Importer.js | 203 ++++++++++++++---------- lib/JsModule.js | 13 +- lib/__tests__/Importer-test.js | 86 +++++++++- lib/__tests__/findMatchingFiles-test.js | 8 +- lib/findJsModulesFor.js | 143 ++++++++++------- lib/findMatchingFiles.js | 51 +++--- lib/importjs.js | 32 ++-- package.json | 1 + 9 files changed, 349 insertions(+), 191 deletions(-) diff --git a/lib/Configuration.js b/lib/Configuration.js index de3a2f14..69bfd383 100644 --- a/lib/Configuration.js +++ b/lib/Configuration.js @@ -248,7 +248,7 @@ export default class Configuration { importPath = importPath.replace(/\{filename\}/, path.basename(pathToCurrentFile, path.extname(pathToCurrentFile))); } - return new JsModule({ importPath }); + return new JsModule({ importPath, variableName }); } resolveNamedExports(variableName: string): ?JsModule { @@ -266,6 +266,7 @@ export default class Configuration { const jsModule = new JsModule({ importPath, hasNamedExports: true, + variableName, }); if (this.get('useRelativePaths', { fromFile: relativeFilePath }) && !relativeFilePath.startsWith('node_modules')) { diff --git a/lib/Importer.js b/lib/Importer.js index c4985780..72c0e505 100644 --- a/lib/Importer.js +++ b/lib/Importer.js @@ -51,54 +51,65 @@ export default class Importer { /** * Imports one variable */ - import(variableName: string): Object { - const jsModule = this.findOneJsModule(variableName); - if (!jsModule) { - this.message(`No JS module to import for \`${variableName}\``); - return this.results(); - } + import(variableName: string): Promise { + return new Promise((resolve: Function, reject: Function) => { + this.findOneJsModule(variableName).then((jsModule: JsModule) => { + if (!jsModule) { + this.message(`No JS module to import for \`${variableName}\``); + resolve(this.results()); + return; + } - const jsModuleName = jsModule.displayName(); - if (jsModule.hasNamedExports) { - this.message(`Imported \`${variableName}\` from \`${jsModuleName}\``); - } else { - this.message(`Imported \`${jsModuleName}\``); - } + const jsModuleName = jsModule.displayName(); + if (jsModule.hasNamedExports) { + this.message(`Imported \`${variableName}\` from \`${jsModuleName}\``); + } else { + this.message(`Imported \`${jsModuleName}\``); + } - const oldImports = this.findCurrentImports(); - const importStatement = jsModule.toImportStatement(variableName, this.config); - oldImports.imports.push(importStatement); - this.replaceImports(oldImports.range, oldImports.imports); + const oldImports = this.findCurrentImports(); + const importStatement = jsModule.toImportStatement(this.config); + oldImports.imports.push(importStatement); + this.replaceImports(oldImports.range, oldImports.imports); - return this.results(); + resolve(this.results()); + }).catch((error: Object) => { + reject(error); + }); + }); } - goto(variableName: string): Object { - const jsModules = findJsModulesFor( - this.config, - variableName, - this.pathToCurrentFile - ); - - const jsModule = this.resolveModuleUsingCurrentImports( - jsModules, variableName); + goto(variableName: string): Promise { + return new Promise((resolve: Function, reject: Function) => { + findJsModulesFor( + this.config, + variableName, + this.pathToCurrentFile + ).then((jsModules: Array) => { + const jsModule = this.resolveModuleUsingCurrentImports( + jsModules, variableName); - if (!jsModule) { - // The current word is not mappable to one of the JS modules that we - // found. This can happen if the user does not select one from the list. - // We have nothing to go to, so we return early. - this.message(`No JS module found for \`${variableName}\``); - return this.results(); - } + if (!jsModule) { + // The current word is not mappable to one of the JS modules that we + // found. This can happen if the user does not select one from the list. + // We have nothing to go to, so we return early. + this.message(`No JS module found for \`${variableName}\``); + resolve(this.results()); + return; + } - const filePath = jsModule.resolvedFilePath(this.pathToCurrentFile); - const results = this.results(); - results.goto = filePath; - return results; + const filePath = jsModule.resolvedFilePath(this.pathToCurrentFile); + const results = this.results(); + results.goto = filePath; + resolve(results); + }).catch((error: Object) => { + reject(error); + }); + }); } // Removes unused imports and adds imports for undefined variables - fixImports(): Object { + fixImports(): Promise { const cli = new eslint.CLIEngine(); const config = cli.getConfigForFile(this.pathToCurrentFile); @@ -160,37 +171,47 @@ export default class Importer { newImports.deleteVariables(unusedImportVariables); const addedVariables = new Set(); - undefinedVariables.forEach((variable: string) => { - const jsModule = this.findOneJsModule(variable); - if (!jsModule) { - return; - } - addedVariables.add(variable); - newImports.push(jsModule.toImportStatement(variable, this.config)); - }); + return new Promise((resolve: Function, reject: Function) => { + const allPromises = []; + undefinedVariables.forEach((variable: string) => { + allPromises.push(this.findOneJsModule(variable)); + }); + Promise.all(allPromises).then((results: Array) => { + results.forEach((jsModule: JsModule) => { + if (!jsModule) { + return; + } + addedVariables.add(jsModule.variableName); + newImports.push(jsModule.toImportStatement(this.config)); + }); - if (!unusedImportVariables.size && !addedVariables.size) { - // return early to avoid unnecessary computation - return this.results(); - } + if (!unusedImportVariables.size && !addedVariables.size) { + // return early to avoid unnecessary computation + resolve(this.results()); + return; + } - this.replaceImports(oldImports.range, newImports); + this.replaceImports(oldImports.range, newImports); - this.message(this.fixImportsMessage( - unusedImportVariables, addedVariables)); + this.message(this.fixImportsMessage( + unusedImportVariables, addedVariables)); - return this.results(); + resolve(this.results()); + }).catch((error: Object) => { + reject(error); + }); + }); } - addImports(imports: Object): Object { + addImports(imports: Object): Promise { const oldImports = this.findCurrentImports(); const newImports = oldImports.imports.clone(); const variables = Object.keys(imports); variables.forEach((variableName: string) => { const importPath = imports[variableName]; - newImports.push(new JsModule({ importPath }).toImportStatement( - variableName, this.config)); + newImports.push(new JsModule( + { importPath, variableName }).toImportStatement(this.config)); }); if (variables.length === 1) { @@ -201,49 +222,62 @@ export default class Importer { this.replaceImports(oldImports.range, newImports); - return this.results(); + return new Promise((resolve: Function) => { + resolve(this.results()); + }); } rewriteImports(): Object { const oldImports = this.findCurrentImports(); const newImports = new ImportStatements(this.config); - oldImports.imports.forEach((imp: ImportStatement) => { - imp.variables().forEach((variable: string) => { - const jsModule = this.resolveModuleUsingCurrentImports( - findJsModulesFor(this.config, variable, this.pathToCurrentFile), - variable - ); + return new Promise((resolve: Function, reject: Function) => { + const variables = []; + oldImports.imports.forEach((imp: ImportStatement) => { + variables.push(...imp.variables()); + }); + const promises = variables.map((variable: string): Promise => + findJsModulesFor(this.config, variable, this.pathToCurrentFile)); - if (!jsModule) { - return; - } + Promise.all(promises).then((results: Array>) => { + results.forEach((jsModules: Array) => { + const jsModule = this.resolveModuleUsingCurrentImports( + jsModules, jsModules[0].variableName); - newImports.push(jsModule.toImportStatement(variable, this.config)); + if (!jsModule) { + return; + } + + newImports.push(jsModule.toImportStatement(this.config)); + }); + this.replaceImports(oldImports.range, newImports); + resolve(this.results()); + }).catch((error: Object) => { + reject(error); }); }); - - this.replaceImports(oldImports.range, newImports); - - return this.results(); } message(str: string) { this.messages.push(str); } - findOneJsModule(variableName: string): ?JsModule { - const jsModules = findJsModulesFor( - this.config, - variableName, - this.pathToCurrentFile - ); - - if (!jsModules.length) { - return null; - } - - return this.resolveOneJsModule(jsModules, variableName); + findOneJsModule(variableName: string): Promise { + return new Promise((resolve: Function, reject: Function) => { + findJsModulesFor( + this.config, + variableName, + this.pathToCurrentFile + ).then((jsModules: Array) => { + if (!jsModules.length) { + resolve(null); + return; + } + resolve(this.resolveOneJsModule(jsModules, variableName)); + }).catch((error: Object) => { + reject(error); + }); + }); } replaceImports(oldImportsRange: Object, newImports: ImportStatements) { @@ -353,6 +387,7 @@ export default class Importer { const matchedModule = new JsModule({ importPath: matchingImportStatement.path, hasNamedExports, + variableName, }); return matchedModule; diff --git a/lib/JsModule.js b/lib/JsModule.js index e306d32f..55800da7 100644 --- a/lib/JsModule.js +++ b/lib/JsModule.js @@ -23,6 +23,7 @@ export default class JsModule { lookupPath: string; filePath: string; mainFile: ?string; + variableName: string; /** * @param {String} opts.lookupPath the lookup path in which this module was @@ -35,6 +36,7 @@ export default class JsModule { * @param {Array} opts.stripFileExtensions a list of file extensions to strip, * e.g. ['.js', '.jsx'] * @param {String} opts.stripFromPath + * @param {String} opts.variableName * @return {JsModule} */ static construct({ @@ -44,6 +46,7 @@ export default class JsModule { relativeFilePath, stripFileExtensions, stripFromPath, + variableName, } : { hasNamedExports?: boolean, lookupPath: string, @@ -51,6 +54,7 @@ export default class JsModule { relativeFilePath: string, stripFileExtensions: Array, stripFromPath?: string, + variableName: string } = {}): ?JsModule { const jsModule = new JsModule(); jsModule.lookupPath = normalizePath(lookupPath); @@ -75,6 +79,7 @@ export default class JsModule { jsModule.importPath = importPath; jsModule.mainFile = mainFile; jsModule.hasNamedExports = hasNamedExports; + jsModule.variableName = variableName; if (makeRelativeTo) { jsModule.makeRelativeTo(makeRelativeTo); } else if (stripFromPath) { @@ -86,12 +91,15 @@ export default class JsModule { constructor({ hasNamedExports, importPath, + variableName, } : { hasNamedExports?: boolean, importPath: string, + variableName: string, } = {}) { this.hasNamedExports = hasNamedExports; this.importPath = importPath; + this.variableName = variableName; } makeRelativeTo(makeRelativeToPath: string) { @@ -167,15 +175,14 @@ export default class JsModule { } toImportStatement( - variableName: string, config: Configuration ): ImportStatement { let namedImports = []; let defaultImport = ''; if (this.hasNamedExports) { - namedImports = [variableName]; + namedImports = [this.variableName]; } else { - defaultImport = variableName; + defaultImport = this.variableName; } const fromFile = this.resolvedFilePath(config.pathToCurrentFile); diff --git a/lib/__tests__/Importer-test.js b/lib/__tests__/Importer-test.js index 85fc08c8..ea3461b7 100644 --- a/lib/__tests__/Importer-test.js +++ b/lib/__tests__/Importer-test.js @@ -1,3 +1,5 @@ +import deasync from 'deasync'; + import FileUtils from '../FileUtils'; import Importer from '../Importer'; import findMatchingFiles from '../findMatchingFiles'; @@ -85,7 +87,21 @@ describe('Importer', () => { beforeEach(() => { subject = () => { setup(); - result = new Importer(text.split('\n'), pathToCurrentFile).import(word); + let error; + let done = false; + new Importer(text.split('\n'), pathToCurrentFile) + .import(word).then((promiseResult) => { + result = promiseResult; + done = true; + }) + .catch((promiseError) => { + error = promiseError; + done = true; + }); + deasync.loopWhile(() => !done); + if (error) { + throw new Error(error); + } return result.fileContent; }; }); @@ -2038,8 +2054,21 @@ foobar beforeEach(() => { subject = () => { setup(); - result = new Importer(text.split('\n'), pathToCurrentFile) - .addImports(resolvedImports); + let error; + let done = false; + new Importer(text.split('\n'), pathToCurrentFile) + .addImports(resolvedImports).then((promiseResult) => { + result = promiseResult; + done = true; + }) + .catch((promiseError) => { + error = promiseError; + done = true; + }); + deasync.loopWhile(() => !done); + if (error) { + throw new Error(error); + } return result.fileContent; }; }); @@ -2101,7 +2130,21 @@ foo; bar; beforeEach(() => { subject = () => { setup(); - result = new Importer(text.split('\n'), pathToCurrentFile).fixImports(); + let error; + let done = false; + new Importer(text.split('\n'), pathToCurrentFile) + .fixImports().then((promiseResult) => { + result = promiseResult; + done = true; + }) + .catch((promiseError) => { + error = promiseError; + done = true; + }); + deasync.loopWhile(() => !done); + if (error) { + throw new Error(error); + } return result.fileContent; }; }); @@ -2556,8 +2599,23 @@ export default function foo() { subject = () => { setup(); - return new Importer(text.split('\n'), pathToCurrentFile). - rewriteImports().fileContent; + let error; + let result; + let done = false; + new Importer(text.split('\n'), pathToCurrentFile) + .rewriteImports().then((promiseResult) => { + result = promiseResult; + done = true; + }) + .catch((promiseError) => { + error = promiseError; + done = true; + }); + deasync.loopWhile(() => !done); + if (error) { + throw new Error(error); + } + return result.fileContent; }; }); @@ -2679,7 +2737,21 @@ bar beforeEach(() => { subject = () => { setup(); - result = new Importer(text.split('\n'), pathToCurrentFile).goto(word); + let error; + let done = false; + new Importer(text.split('\n'), pathToCurrentFile) + .goto(word).then((promiseResult) => { + result = promiseResult; + done = true; + }) + .catch((promiseError) => { + error = promiseError; + done = true; + }); + deasync.loopWhile(() => !done); + if (error) { + throw new Error(error); + } return result.goto; }; }); diff --git a/lib/__tests__/findMatchingFiles-test.js b/lib/__tests__/findMatchingFiles-test.js index 66e0c3ef..745acdd8 100644 --- a/lib/__tests__/findMatchingFiles-test.js +++ b/lib/__tests__/findMatchingFiles-test.js @@ -1,6 +1,7 @@ import fs from 'fs'; import path from 'path'; +import deasync from 'deasync'; import mkdirp from 'mkdirp'; import rimraf from 'rimraf'; @@ -43,7 +44,12 @@ import findMatchingFiles from '../findMatchingFiles'; fs.closeSync(fs.openSync(fullPath, 'w')); // create empty file }); - return findMatchingFiles(lookupPath, word, finder); + let result; + findMatchingFiles(lookupPath, word, finder).then((files) => { + result = files; + }); + deasync.loopWhile(() => !result); + return result; }; }); diff --git a/lib/findJsModulesFor.js b/lib/findJsModulesFor.js index 95bb8981..af4ffe50 100644 --- a/lib/findJsModulesFor.js +++ b/lib/findJsModulesFor.js @@ -16,7 +16,10 @@ function findImportsFromEnvironment( ): Array { return config.environmentCoreModules() .filter((dep: string): boolean => dep.toLowerCase() === variableName.toLowerCase()) - .map((dep: string): JsModule => new JsModule({ importPath: dep })); + .map((dep: string): JsModule => new JsModule({ + importPath: dep, + variableName, + })); } function compact(arr: Array): Array { @@ -49,6 +52,7 @@ function findImportsFromPackageJson( lookupPath: 'node_modules', relativeFilePath: `node_modules/${dep}/package.json`, stripFileExtensions: [], + variableName, }) )); @@ -59,73 +63,94 @@ function findImportsFromLocalFiles( config: Configuration, variableName: string, pathToCurrentFile: string -): Array { - const matchedModules = []; - - config.get('lookupPaths').forEach((lookupPath: string) => { - findMatchingFiles(lookupPath, variableName).forEach((f: string) => { - const isExcluded = config.get('excludes') - .some((globPattern: string): boolean => minimatch(f, globPattern)); - if (isExcluded) { - return; - } - - const module = JsModule.construct({ - lookupPath, - relativeFilePath: f, - stripFileExtensions: - config.get('stripFileExtensions', { fromFile: f }), - makeRelativeTo: - config.get('useRelativePaths', { fromFile: f }) && - pathToCurrentFile, - stripFromPath: config.get('stripFromPath', { fromFile: f }), +): Promise { + return new Promise((resolve: Function, reject: Function) => { + const lookupPaths = config.get('lookupPaths'); + const promises = lookupPaths.map((lookupPath: string): Promise => + findMatchingFiles(lookupPath, variableName)); + + Promise.all(promises).then((results: Array>) => { + const matchedModules = []; + results.forEach((files: Array, index: number) => { + // Grab the lookup path originally associated with the promise. Because + // Promise.all maintains the order of promises, we can use the index + // here. + const lookupPath = lookupPaths[index]; + files.forEach((f: string) => { + const isExcluded = config.get('excludes') + .some((globPattern: string): boolean => minimatch(f, globPattern)); + if (isExcluded) { + return; + } + + const module = JsModule.construct({ + lookupPath, + relativeFilePath: f, + stripFileExtensions: + config.get('stripFileExtensions', { fromFile: f }), + makeRelativeTo: + config.get('useRelativePaths', { fromFile: f }) && + pathToCurrentFile, + stripFromPath: config.get('stripFromPath', { fromFile: f }), + variableName, + }); + + if (module) { + matchedModules.push(module); + } + }); }); - - if (module) { - matchedModules.push(module); - } + resolve(matchedModules); + }).catch((error: Object) => { + reject(error); }); }); - - return matchedModules; } export default function findJsModulesFor( config: Configuration, variableName: string, pathToCurrentFile: string -): Array { - const aliasModule = config.resolveAlias(variableName, pathToCurrentFile); - if (aliasModule) { - return [aliasModule]; - } - - const namedImportsModule = config.resolveNamedExports(variableName); - if (namedImportsModule) { - return [namedImportsModule]; - } - - let matchedModules = []; +): Promise { + return new Promise((resolve: Function, reject: Function) => { + const aliasModule = config.resolveAlias(variableName, pathToCurrentFile); + if (aliasModule) { + resolve([aliasModule]); + return; + } - matchedModules.push(...findImportsFromEnvironment(config, variableName)); - matchedModules.push(...findImportsFromPackageJson(config, variableName)); - matchedModules.push( - ...findImportsFromLocalFiles(config, variableName, pathToCurrentFile) - ); + const namedImportsModule = config.resolveNamedExports(variableName); + if (namedImportsModule) { + resolve([namedImportsModule]); + return; + } - // If you have overlapping lookup paths, you might end up seeing the same - // module to import twice. In order to dedupe these, we remove the module - // with the longest path - matchedModules = sortBy( - matchedModules, - (module: JsModule): number => module.importPath.length - ); - matchedModules = uniqBy( - matchedModules, - (module: JsModule): string => module.filePath - ); - return sortBy( - matchedModules, - (module: JsModule): string => module.displayName() - ); + let matchedModules = []; + + matchedModules.push(...findImportsFromEnvironment(config, variableName)); + matchedModules.push(...findImportsFromPackageJson(config, variableName)); + + findImportsFromLocalFiles( + config, variableName, pathToCurrentFile).then((modules: Array) => { + matchedModules.push(...modules); + + // If you have overlapping lookup paths, you might end up seeing the same + // module to import twice. In order to dedupe these, we remove the module + // with the longest path + matchedModules = sortBy( + matchedModules, + (module: JsModule): number => module.importPath.length + ); + matchedModules = uniqBy( + matchedModules, + (module: JsModule): string => module.filePath + ); + resolve(sortBy( + matchedModules, + (module: JsModule): string => module.displayName() + )); + }).catch((error: Object) => { + reject(error); + }); + }); } diff --git a/lib/findMatchingFiles.js b/lib/findMatchingFiles.js index f586135a..409a1814 100644 --- a/lib/findMatchingFiles.js +++ b/lib/findMatchingFiles.js @@ -8,7 +8,7 @@ import formattedToRegex from './formattedToRegex'; function findMatchingFilesWithFind( lookupPath: string, validFilesRegex: string -): Array { +): Promise> { const findCommand = [ `find ${lookupPath}`, '-name "**.js*"', @@ -16,32 +16,37 @@ function findMatchingFilesWithFind( ].join(' '); const command = `${findCommand} | egrep -i \"${validFilesRegex}\"`; - // TODO switch to spawn so we can start processing the stream as it comes - // in. - let out = ''; - let err = ''; - try { - out = String(childProcess.execSync(command)); - } catch (error) { - err = String(error.stderr); - } - - if (err !== '') { - throw new Error(err); - } - - // Filter out empty lines before returning - return out.split('\n').filter((file: string): boolean => !!file); + return new Promise((resolve: Function, reject: Function) => { + // TODO switch to spawn so we can start processing the stream as it comes + // in. + childProcess.exec(command, (err: Object, stdout: string, stderr: string) => { + if (String(stderr) !== '') { + reject(String(stderr)); + return; + } + // Filter out empty lines before returning + resolve(String(stdout).split('\n').filter( + (file: string): boolean => !!file)); + }); + }); } function findMatchingFilesWithNode( lookupPath: string, validFilesRegex: string -): Array { - return glob.sync(`${lookupPath}/**/*.js*`, { - ignore: './node_modules/**', - }).filter((filePath: string): bool => - new RegExp(validFilesRegex, 'i').test(filePath)); +): Promise> { + return new Promise((resolve: Function, reject: Function) => { + glob(`${lookupPath}/**/*.js*`, { + ignore: './node_modules/**', + }, (err: Object, result: Array) => { + if (err) { + reject(err); + return; + } + resolve(result.filter((filePath: string): bool => + new RegExp(validFilesRegex, 'i').test(filePath))); + }); + }); } /** @@ -50,7 +55,7 @@ function findMatchingFilesWithNode( export default function findMatchingFiles( lookupPath: string, variableName: string -): Array { +): Promise> { if (lookupPath === '') { // If lookupPath is an empty string, the `find` command will not work // as desired so we bail early. diff --git a/lib/importjs.js b/lib/importjs.js index 3cccf58d..5e7b3927 100644 --- a/lib/importjs.js +++ b/lib/importjs.js @@ -42,14 +42,18 @@ function runCommand( ) { getLines(pathToFile, (lines: Array) => { const importer = new Importer(lines, pathToFile); - const result = executor(importer); - if (overwrite) { - fs.writeFile(pathToFile, result.fileContent, (err: Error) => { - if (err) throw err; - }); - } else { - stdoutWrite(JSON.stringify(result)); - } + executor(importer).then((result: Object) => { + if (overwrite) { + fs.writeFile(pathToFile, result.fileContent, (err: Error) => { + if (err) throw err; + }); + } else { + stdoutWrite(JSON.stringify(result)); + } + }).catch((error: Object) => { + console.error(error); // eslint-disable-line no-console + process.exit(1); + }); }); } @@ -66,21 +70,21 @@ const sharedOptions = { program.command('word ') .option(...sharedOptions.overwrite) .action((word: string, pathToFile: string, options: Object) => { - const executor = (importer: Importer): Object => importer.import(word); + const executor = (importer: Importer): Promise => importer.import(word); runCommand(executor, pathToFile, options); }); program.command('fix ') .option(...sharedOptions.overwrite) .action((pathToFile: string, options: Object) => { - const executor = (importer: Importer): Object => importer.fixImports(); + const executor = (importer: Importer): Promise => importer.fixImports(); runCommand(executor, pathToFile, options); }); program.command('rewrite ') .option(...sharedOptions.overwrite) .action((pathToFile: string, options: Object) => { - const executor = (importer: Importer): Object => importer.rewriteImports(); + const executor = (importer: Importer): Promise => importer.rewriteImports(); runCommand(executor, pathToFile, options); }); @@ -88,14 +92,16 @@ program.command('add ') .option(...sharedOptions.overwrite) .action((imports: string, pathToFile: string, options: Object) => { const executor = - (importer: Importer): Object => importer.addImports(JSON.parse(imports)); + (importer: Importer): Promise => importer.addImports(JSON.parse(imports)); runCommand(executor, pathToFile, options); }); program.command('goto ') .action((word: string, pathToFile: string) => { getLines(pathToFile, (lines: Array) => { - stdoutWrite(JSON.stringify(new Importer(lines, pathToFile).goto(word))); + new Importer(lines, pathToFile).goto(word).then((result: Object) => { + stdoutWrite(JSON.stringify(result)); + }); }); }); diff --git a/package.json b/package.json index 6c7589ea..4bb2029c 100644 --- a/package.json +++ b/package.json @@ -47,6 +47,7 @@ "babel-plugin-transform-class-properties": "^6.8.0", "babel-plugin-transform-flow-strip-types": "^6.8.0", "babel-preset-es2015": "^6.6.0", + "deasync": "^0.1.7", "eslint-config-airbnb-base": "^3.0.1", "eslint-plugin-flow-vars": "^0.4.0", "eslint-plugin-flowtype": "^2.2.7", From b53eee560c2e897a4d729876619681882de659d5 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Tue, 14 Jun 2016 20:32:47 +0200 Subject: [PATCH 02/41] Lock babylon at 6.8.0 I kept getting an error in Travis from eslint: import-js/lib/findJsModulesFor.js 0:0 error Parsing error: Object.defineProperty called on non-object Initially, I wasn't getting this error locally. But after wiping node_modules and reinstalling, I saw it there too. After some googling, I came across this recent babel-eslint issue: https://github.com/babel/babel-eslint/issues/321 In that thread, people suggest pinning babylon at 6.8.0 (a regression was introduced in 6.8.1). Doing that seems to do the trick. --- package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/package.json b/package.json index 4bb2029c..cb9b9812 100644 --- a/package.json +++ b/package.json @@ -47,6 +47,7 @@ "babel-plugin-transform-class-properties": "^6.8.0", "babel-plugin-transform-flow-strip-types": "^6.8.0", "babel-preset-es2015": "^6.6.0", + "babylon": "6.8.0", "deasync": "^0.1.7", "eslint-config-airbnb-base": "^3.0.1", "eslint-plugin-flow-vars": "^0.4.0", From 808fc7caf5257c603c99f0b3c8673064069b6147 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Thu, 16 Jun 2016 20:17:26 +0200 Subject: [PATCH 03/41] Make importjs.js a function that takes argv This is in preparation of adding a importjs command that will run in the background, and allow commands from stdin as well as directly on the command-line. --- bin/importjs.js | 2 +- lib/importjs.js | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/bin/importjs.js b/bin/importjs.js index 554b33e8..b264a7a7 100755 --- a/bin/importjs.js +++ b/bin/importjs.js @@ -18,4 +18,4 @@ const oldPath = process.env.NODE_PATH; process.env.NODE_PATH = `${oldPath || ''}:${process.cwd()}/node_modules/`; require('module').Module._initPaths(); // eslint-disable-line no-underscore-dangle -require('../build/importjs.js'); +require('../build/importjs.js')(process.argv); diff --git a/lib/importjs.js b/lib/importjs.js index 5e7b3927..1ed5061f 100644 --- a/lib/importjs.js +++ b/lib/importjs.js @@ -122,8 +122,10 @@ program.on('--help', () => { stdoutWrite(''); }); -program.parse(process.argv); +export default function importjs(argv: Array) { + program.parse(argv); -if (!process.argv.slice(2).length) { - program.outputHelp(); + if (!argv.slice(2).length) { + program.outputHelp(); + } } From 5ebc5ba76147c97a10c7689c3d3657ee79ea40b5 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Thu, 16 Jun 2016 20:37:09 +0200 Subject: [PATCH 04/41] Move fixing NODE_PATH into own module This is so that I can reuse it in the daemon script that I'm currently writing. --- bin/importjs.js | 19 +------------------ lib/fix-node-path.js | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 18 deletions(-) create mode 100644 lib/fix-node-path.js diff --git a/bin/importjs.js b/bin/importjs.js index b264a7a7..87c0f672 100755 --- a/bin/importjs.js +++ b/bin/importjs.js @@ -1,21 +1,4 @@ #!/usr/bin/env node -/* eslint-disable strict */ - -'use strict'; - -// This is hacky, but necessary to make eslint find plugins local to the files -// being linted. Without it, you'll get an error message of the following kind: -// -// Error: Cannot find module 'eslint-config-brigade' -// -// This is because eslint will look for modules relative to where it installed. -// The eslint we are using is local to import-js, so any plugin referenced for -// the file we are linting will have to be looked up relative to that file. -// -// Technique from http://stackoverflow.com/questions/11969175 -const oldPath = process.env.NODE_PATH; -process.env.NODE_PATH = `${oldPath || ''}:${process.cwd()}/node_modules/`; -require('module').Module._initPaths(); // eslint-disable-line no-underscore-dangle - +require('../build/fix-node-path'); require('../build/importjs.js')(process.argv); diff --git a/lib/fix-node-path.js b/lib/fix-node-path.js new file mode 100644 index 00000000..69d462e1 --- /dev/null +++ b/lib/fix-node-path.js @@ -0,0 +1,14 @@ +// This is hacky, but necessary to make eslint find plugins local to the files +// being linted. Without it, you'll get an error message of the following kind: +// +// Error: Cannot find module 'eslint-config-brigade' +// +// This is because eslint will look for modules relative to where it installed. +// The eslint we are using is local to import-js, so any plugin referenced for +// the file we are linting will have to be looked up relative to that file. +// +// Technique from http://stackoverflow.com/questions/11969175 +const oldPath = process.env.NODE_PATH; +process.env.NODE_PATH = `${oldPath || ''}:${process.cwd()}/node_modules/`; +require('module').Module._initPaths(); // eslint-disable-line no-underscore-dangle + From 3daf870338ba6f97834c95c5a88a6385fb4acac8 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Thu, 16 Jun 2016 21:01:18 +0200 Subject: [PATCH 05/41] Add simple daemon It's not useful yet, but I'm using it as a POC that I can write to STDIN, react to input, then write to STDOUT. The current version simply prints the same output you provide. Here's how to test it in mac osx: Create a pipe that we can use to communicate with: > mkfifo IMPORTJS Start the daemon using the pipe we created as STDIN: > importjsd < IMPORTJS Write to the pipe: > echo 'galooshi' > IMPORTJS Watch for 'galooshi' to get written to the console where the daemon is running. --- bin/importjsd.js | 4 ++++ lib/daemon.js | 13 +++++++++++++ package.json | 3 ++- 3 files changed, 19 insertions(+), 1 deletion(-) create mode 100755 bin/importjsd.js create mode 100644 lib/daemon.js diff --git a/bin/importjsd.js b/bin/importjsd.js new file mode 100755 index 00000000..e33b1a9a --- /dev/null +++ b/bin/importjsd.js @@ -0,0 +1,4 @@ +#!/usr/bin/env node + +require('../build/fix-node-path'); +require('../build/daemon.js')(); diff --git a/lib/daemon.js b/lib/daemon.js new file mode 100644 index 00000000..d1f3c5f5 --- /dev/null +++ b/lib/daemon.js @@ -0,0 +1,13 @@ +import readline from 'readline'; + +export default function daemon() { + const rlInterface = readline.createInterface({ + input: process.stdin, + output: process.stdout, + terminal: false, + }); + + rlInterface.on('line', (str: string) => { + console.log('You just typed: ' + str); + }); +} diff --git a/package.json b/package.json index cb9b9812..a7fa9ae2 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,8 @@ "version": "0.8.0", "description": "Simplifies importing JavaScript modules", "bin": { - "importjs": "./bin/importjs.js" + "importjs": "./bin/importjs.js", + "importjsd": "./bin/importjsd.js" }, "main": "build/Importer.js", "scripts": { From 8f3b5e5d6e8f129ea0852e53dc5a7f5e4d410b5f Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Thu, 16 Jun 2016 22:14:38 +0200 Subject: [PATCH 06/41] Make daemon use Importer This brings us closer to having importjsd work as a real daemon for importjs. --- lib/daemon.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/daemon.js b/lib/daemon.js index d1f3c5f5..6a7f7bef 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -1,5 +1,7 @@ import readline from 'readline'; +import Importer from './Importer'; + export default function daemon() { const rlInterface = readline.createInterface({ input: process.stdin, @@ -7,7 +9,17 @@ export default function daemon() { terminal: false, }); - rlInterface.on('line', (str: string) => { - console.log('You just typed: ' + str); + rlInterface.on('line', (jsonCommand: string) => { + const command = JSON.parse(jsonCommand); + const importer = new Importer( + command.fileContent.split('\n'), command.pathToFile); + + // TODO: don't automatically invoke function here because of security issues + importer[command.command](...command.args).then((result) => { + process.stdout.write(`${JSON.stringify(result)}\n`); + }).catch((error) => { + // TODO: print entire stack trace here + process.stderr.write(error.toString()); + }); }); } From f9b5cdd749eb542f3d378342f05fd70706305e2d Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Fri, 17 Jun 2016 21:38:45 +0200 Subject: [PATCH 07/41] Map command names to function names before invocation This is still not great, but I wanted to avoid having to rename commands on the clients. So instead we match the name of the command to its corresponding Importer function. --- lib/daemon.js | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/lib/daemon.js b/lib/daemon.js index 6a7f7bef..28725311 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -2,6 +2,14 @@ import readline from 'readline'; import Importer from './Importer'; +const commandsToFunctionNames = { + fix: 'fixImports', + word: 'import', + goto: 'goto', + rewrite: 'rewriteImports', + add: 'addImports', +}; + export default function daemon() { const rlInterface = readline.createInterface({ input: process.stdin, @@ -9,13 +17,14 @@ export default function daemon() { terminal: false, }); - rlInterface.on('line', (jsonCommand: string) => { - const command = JSON.parse(jsonCommand); + rlInterface.on('line', (jsonPayload: string) => { + const payload = JSON.parse(jsonPayload); const importer = new Importer( - command.fileContent.split('\n'), command.pathToFile); + payload.fileContent.split('\n'), payload.pathToFile); // TODO: don't automatically invoke function here because of security issues - importer[command.command](...command.args).then((result) => { + const functionName = commandsToFunctionNames[payload.command]; + importer[functionName](payload.commandArg).then((result) => { process.stdout.write(`${JSON.stringify(result)}\n`); }).catch((error) => { // TODO: print entire stack trace here From 5c104409305efe16dbffd04c5041c2d88e55846a Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Sat, 18 Jun 2016 20:13:29 +0200 Subject: [PATCH 08/41] Print importjsd errors to stdout I'm playing around with using a vim channel to communicate with the daemon. It's a lot simpler if we can assume all results (even errors) get printed to stdout. --- lib/daemon.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/daemon.js b/lib/daemon.js index 28725311..fc90c85e 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -28,7 +28,9 @@ export default function daemon() { process.stdout.write(`${JSON.stringify(result)}\n`); }).catch((error) => { // TODO: print entire stack trace here - process.stderr.write(error.toString()); + process.stdout.write(JSON.stringify({ + error: error.toString(), + })); }); }); } From c23f42c456fe40e311447ce322354a397a0f5f57 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Sat, 18 Jun 2016 20:31:30 +0200 Subject: [PATCH 09/41] Add some logging to daemon For now, this will always be on and will write to /tmp/importjsd.log. I intend to keep things this way only during development. After that we will probably need a more sophisticated logging setup. --- lib/daemon.js | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/lib/daemon.js b/lib/daemon.js index fc90c85e..b07b30c4 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -1,3 +1,4 @@ +import fs from 'fs'; import readline from 'readline'; import Importer from './Importer'; @@ -10,7 +11,17 @@ const commandsToFunctionNames = { add: 'addImports', }; +function log(message) { + fs.appendFile('/tmp/importjsd.log', `${message}\n`, (err) => { + if (err) { + throw new Error(err); + } + }); +} + export default function daemon() { + log('STARTING daemon'); + const rlInterface = readline.createInterface({ input: process.stdin, output: process.stdout, @@ -18,19 +29,25 @@ export default function daemon() { }); rlInterface.on('line', (jsonPayload: string) => { + log(`RECEIVED payload: ${jsonPayload}`); + // The json payload is an array containing a job id and job details. const payload = JSON.parse(jsonPayload); const importer = new Importer( payload.fileContent.split('\n'), payload.pathToFile); // TODO: don't automatically invoke function here because of security issues const functionName = commandsToFunctionNames[payload.command]; - importer[functionName](payload.commandArg).then((result) => { - process.stdout.write(`${JSON.stringify(result)}\n`); - }).catch((error) => { + importer[functionName](payload.commandArg).then((result: Object) => { + const jsonResponse = JSON.stringify(result); + log(`SENDING response: ${jsonResponse}`); + process.stdout.write(`${jsonResponse}\n`); + }).catch((error: Object) => { // TODO: print entire stack trace here - process.stdout.write(JSON.stringify({ + const jsonResponse = JSON.stringify({ error: error.toString(), - })); + }); + log(`ERROR response: ${jsonResponse}`); + process.stdout.write(`${jsonResponse}\n`); }); }); } From c93cafe4437b3d279b216808aa249cdb059e15a8 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Sat, 18 Jun 2016 21:49:39 +0200 Subject: [PATCH 10/41] Send error stack trace in the daemon response This will help track down failures better. --- lib/daemon.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/daemon.js b/lib/daemon.js index b07b30c4..d1b81cfa 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -42,9 +42,8 @@ export default function daemon() { log(`SENDING response: ${jsonResponse}`); process.stdout.write(`${jsonResponse}\n`); }).catch((error: Object) => { - // TODO: print entire stack trace here const jsonResponse = JSON.stringify({ - error: error.toString(), + error: error.stack, }); log(`ERROR response: ${jsonResponse}`); process.stdout.write(`${jsonResponse}\n`); From 8136298d8cf24cb3d49d1ca451a5ef6c22f21aad Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Sat, 18 Jun 2016 21:50:53 +0200 Subject: [PATCH 11/41] Prepend daemon log messages with timestamp This will help when looking at the logfile at a later time. --- lib/daemon.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/daemon.js b/lib/daemon.js index d1b81cfa..2ed8618f 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -12,11 +12,12 @@ const commandsToFunctionNames = { }; function log(message) { - fs.appendFile('/tmp/importjsd.log', `${message}\n`, (err) => { - if (err) { - throw new Error(err); - } - }); + fs.appendFile( + '/tmp/importjsd.log', `${new Date().getTime()}: ${message}\n`, (err) => { + if (err) { + throw new Error(err); + } + }); } export default function daemon() { From 8ebb506c22b61568d93f277e5554c43f6c2a228b Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Sun, 19 Jun 2016 21:39:47 +0200 Subject: [PATCH 12/41] Add watchman-powered file finder This is not yet completely finalized, but I wanted to get something simple out there to start with. The new file finder is only used in the daemon so far, since there's a startup cost involved in using the watchman file finder. https://facebook.github.io/watchman/ --- lib/WatchmanFileCache.js | 122 +++++++++++++++++++++++++++++++++++++++ lib/daemon.js | 9 ++- lib/findMatchingFiles.js | 28 ++++++++- package.json | 1 + 4 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 lib/WatchmanFileCache.js diff --git a/lib/WatchmanFileCache.js b/lib/WatchmanFileCache.js new file mode 100644 index 00000000..bc6b5d23 --- /dev/null +++ b/lib/WatchmanFileCache.js @@ -0,0 +1,122 @@ +import fbWatchman from 'fb-watchman'; + +const SUBSCRIPTION_NAME = 'import-js-subscription'; + +const CACHE = new Set(); + +// Flag to quickly check if the cache is enabled. +let enabled = false; + +function subscribe({ client, fbWatch, relativePath }): Promise { + const subscription = { + // Match any `.js` file + expression: ['allof', ['match', '*.js*']], + // We're only interested in the file name + fields: ['name', 'exists'], + relative_root: relativePath, + }; + + return new Promise((resolve, reject) => { + client.command(['subscribe', fbWatch, SUBSCRIPTION_NAME, subscription], + (error) => { + if (error) { + reject(error); + return; + } + resolve(); + }); + + // Subscription results are emitted via the subscription event. + // Note that this emits for all subscriptions. If you have + // subscriptions with different `fields` you will need to check + // the subscription name and handle the differing data accordingly. + // `resp` looks like this in practice: + // + // { root: '/private/tmp/foo', + // subscription: 'mysubscription', + // files: [ { name: 'node_modules/fb-watchman/index.js', + // size: 4768, + // exists: true, + // type: 'f' } ] } + client.on('subscription', (resp) => { + if (resp.subscription !== SUBSCRIPTION_NAME) { + return; + } + // Flag that we have a working subscription and that files are available. + enabled = true; + + const filteredFiles = resp.files.filter((file) => + !file.name.startsWith('node_modules/')); + filteredFiles.forEach((file) => { + if (file.exists) { + CACHE.add(file.name); + } else { + CACHE.delete(file.name); + } + }); + }); + }); +} + +function startSubscription({ client }): Promise { + return new Promise((resolve, reject) => { + client.command(['watch-project', process.cwd()], (error, resp) => { + if (error) { + reject(error); + return; + } + + // It is considered to be best practice to show any 'warning' or + // 'error' information to the user, as it may suggest steps + // for remediation + if ('warning' in resp) { + console.log('warning: ', resp.warning); + } + + // `watch-project` can consolidate the watch for your + // dir_of_interest with another watch at a higher level in the + // tree, so it is very important to record the `relative_path` + // returned in resp + console.log('watch established on', resp.watch, + 'relative_path', resp.relative_path); + + subscribe({ + client, + fbWatch: resp.watch, + relativePath: resp.relativePath, + }).then(resolve).catch(reject); + }); + }); +} + +export default { + /** + * Get all files from the watchman-powered cache. Returns a promise that will + * resolve if watchman is available, and the file cache is enabled. Will + * resolve immediately if previously initialized. + */ + initialize(): Promise { + return new Promise((resolve, reject) => { + const client = new fbWatchman.Client(); + client.capabilityCheck({ + optional: [], + required: ['relative_root'], + }, (error) => { + if (error) { + client.end(); + reject(error); + } else { + startSubscription({ client }).then(resolve).catch(reject); + } + }); + }); + }, + + isEnabled(): bool { + return enabled; + }, + + getFiles(): Set { + return CACHE; + }, +}; diff --git a/lib/daemon.js b/lib/daemon.js index 2ed8618f..c6ddf789 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -2,6 +2,7 @@ import fs from 'fs'; import readline from 'readline'; import Importer from './Importer'; +import WatchmanFileCache from './WatchmanFileCache'; const commandsToFunctionNames = { fix: 'fixImports', @@ -12,8 +13,9 @@ const commandsToFunctionNames = { }; function log(message) { + const timestamp = new Date().getTime(); fs.appendFile( - '/tmp/importjsd.log', `${new Date().getTime()}: ${message}\n`, (err) => { + '/tmp/importjsd.log', `${timestamp}: ${message}\n`, (err) => { if (err) { throw new Error(err); } @@ -22,6 +24,11 @@ function log(message) { export default function daemon() { log('STARTING daemon'); + WatchmanFileCache.initialize().then(() => { + log('WATCHMAN file cache is enabled'); + }).catch((err: Object) => { + log(`WATCHMAN file cache is not available. Error: ${err.stack}`); + }); const rlInterface = readline.createInterface({ input: process.stdin, diff --git a/lib/findMatchingFiles.js b/lib/findMatchingFiles.js index 409a1814..5d7008aa 100644 --- a/lib/findMatchingFiles.js +++ b/lib/findMatchingFiles.js @@ -1,9 +1,11 @@ // @flow import childProcess from 'child_process'; + import glob from 'glob'; import formattedToRegex from './formattedToRegex'; +import WatchmanFileCache from './WatchmanFileCache'; function findMatchingFilesWithFind( lookupPath: string, @@ -49,6 +51,26 @@ function findMatchingFilesWithNode( }); } +function findMatchingFilesWithWatchman( + lookupPath: string, + validFilesRegex: string +): Promise> { + const tester = new RegExp(validFilesRegex, 'i'); + return new Promise((resolve: Function) => { + const matches = []; + // `getFiles()` returns a Set, so we can't use `filter` here. + WatchmanFileCache.getFiles().forEach((filePath: string) => { + if (!filePath.startsWith(lookupPath)) { + return; + } + if (tester.test(filePath)) { + matches.push(filePath); + } + }); + resolve(matches); + }); +} + /** * Finds files from the local file system matching the variable name. */ @@ -65,7 +87,11 @@ export default function findMatchingFiles( const formattedVarName = formattedToRegex(variableName); const validFilesRegex = `(/|^)${formattedVarName}(/index)?(/package)?\\.js.*`; - if (/^win/.test(process.platform) || process.env.IMPORT_JS_USE_NODE_FINDER) { + if (WatchmanFileCache.isEnabled()) { + return findMatchingFilesWithWatchman(lookupPath, validFilesRegex); + } + if (/^win/.test(process.platform) || + process.env.IMPORT_JS_USE_NODE_FINDER) { return findMatchingFilesWithNode(lookupPath, validFilesRegex); } return findMatchingFilesWithFind(lookupPath, validFilesRegex); diff --git a/package.json b/package.json index a7fa9ae2..a5b8e58c 100644 --- a/package.json +++ b/package.json @@ -64,6 +64,7 @@ "StringScanner": "0.0.3", "commander": "^2.9.0", "eslint": "^2.8.0", + "fb-watchman": "^1.9.0", "glob": "^7.0.3", "lodash.escaperegexp": "^4.1.1", "lodash.flattendeep": "^4.1.0", From 84a54ff521793d579dc172cd26b811605da5a39f Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Mon, 20 Jun 2016 21:17:22 +0200 Subject: [PATCH 13/41] Extract `normalizePath` function into own module I need this function to normalize paths coming from watchman subscription events. That change will come in a later commit. --- lib/Configuration.js | 13 +------------ lib/normalizePath.js | 12 ++++++++++++ 2 files changed, 13 insertions(+), 12 deletions(-) create mode 100644 lib/normalizePath.js diff --git a/lib/Configuration.js b/lib/Configuration.js index 69bfd383..0524279d 100644 --- a/lib/Configuration.js +++ b/lib/Configuration.js @@ -2,12 +2,12 @@ import path from 'path'; -import escapeRegExp from 'lodash.escaperegexp'; import minimatch from 'minimatch'; import semver from 'semver'; import FileUtils from './FileUtils'; import JsModule from './JsModule'; +import normalizePath from './normalizePath'; import requireResolve from './requireResolve'; import version from './version'; @@ -146,17 +146,6 @@ function checkForUnknownConfiguration(config: Object): Array { return messages; } -function normalizePath(rawPath: string): string { - if (!rawPath) { - return './'; - } - const normalized = rawPath.replace(RegExp(`^${escapeRegExp(process.cwd())}`), '.'); - if (normalized.startsWith('.')) { - return normalized; - } - return `./${normalized}`; -} - /** * Checks that the current version is bigger than the `minimumVersion` * defined in config. diff --git a/lib/normalizePath.js b/lib/normalizePath.js new file mode 100644 index 00000000..3ec96645 --- /dev/null +++ b/lib/normalizePath.js @@ -0,0 +1,12 @@ +import escapeRegExp from 'lodash.escaperegexp'; + +export default function normalizePath(rawPath: string): string { + if (!rawPath) { + return './'; + } + const normalized = rawPath.replace(RegExp(`^${escapeRegExp(process.cwd())}`), '.'); + if (normalized.startsWith('.')) { + return normalized; + } + return `./${normalized}`; +} From c02cca11be3ee13e215ad4b3bee6753c39edcd99 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Mon, 20 Jun 2016 21:19:57 +0200 Subject: [PATCH 14/41] Clean up WatchmanFileCache a little - Remove copy-paste comments in this file (from watchman tutorial) - Use `log` (new module extracted from daemon.js) to log stuff - Normalize paths before putting them in the cache (this fixes an issue with `excludes` config being ignored for findFilesWithWatchman). --- lib/WatchmanFileCache.js | 34 +++++++++------------------------- lib/daemon.js | 12 +----------- lib/log.js | 11 +++++++++++ 3 files changed, 21 insertions(+), 36 deletions(-) create mode 100644 lib/log.js diff --git a/lib/WatchmanFileCache.js b/lib/WatchmanFileCache.js index bc6b5d23..e4f01397 100644 --- a/lib/WatchmanFileCache.js +++ b/lib/WatchmanFileCache.js @@ -1,5 +1,8 @@ import fbWatchman from 'fb-watchman'; +import log from './log'; +import normalizePath from './normalizePath'; + const SUBSCRIPTION_NAME = 'import-js-subscription'; const CACHE = new Set(); @@ -26,18 +29,6 @@ function subscribe({ client, fbWatch, relativePath }): Promise { resolve(); }); - // Subscription results are emitted via the subscription event. - // Note that this emits for all subscriptions. If you have - // subscriptions with different `fields` you will need to check - // the subscription name and handle the differing data accordingly. - // `resp` looks like this in practice: - // - // { root: '/private/tmp/foo', - // subscription: 'mysubscription', - // files: [ { name: 'node_modules/fb-watchman/index.js', - // size: 4768, - // exists: true, - // type: 'f' } ] } client.on('subscription', (resp) => { if (resp.subscription !== SUBSCRIPTION_NAME) { return; @@ -48,10 +39,13 @@ function subscribe({ client, fbWatch, relativePath }): Promise { const filteredFiles = resp.files.filter((file) => !file.name.startsWith('node_modules/')); filteredFiles.forEach((file) => { + const normalizedPath = normalizePath(file.name); if (file.exists) { - CACHE.add(file.name); + log(`ADDING file to WatchmanFileCache: ${normalizedPath}`); + CACHE.add(normalizedPath); } else { - CACHE.delete(file.name); + log(`REMOVING file from WatchmanFileCache: ${normalizedPath}`); + CACHE.delete(normalizedPath); } }); }); @@ -66,20 +60,10 @@ function startSubscription({ client }): Promise { return; } - // It is considered to be best practice to show any 'warning' or - // 'error' information to the user, as it may suggest steps - // for remediation if ('warning' in resp) { - console.log('warning: ', resp.warning); + log(`WARNING received during watchman init: ${resp.warning}`); } - // `watch-project` can consolidate the watch for your - // dir_of_interest with another watch at a higher level in the - // tree, so it is very important to record the `relative_path` - // returned in resp - console.log('watch established on', resp.watch, - 'relative_path', resp.relative_path); - subscribe({ client, fbWatch: resp.watch, diff --git a/lib/daemon.js b/lib/daemon.js index c6ddf789..e7998f0f 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -1,8 +1,8 @@ -import fs from 'fs'; import readline from 'readline'; import Importer from './Importer'; import WatchmanFileCache from './WatchmanFileCache'; +import log from './log'; const commandsToFunctionNames = { fix: 'fixImports', @@ -12,16 +12,6 @@ const commandsToFunctionNames = { add: 'addImports', }; -function log(message) { - const timestamp = new Date().getTime(); - fs.appendFile( - '/tmp/importjsd.log', `${timestamp}: ${message}\n`, (err) => { - if (err) { - throw new Error(err); - } - }); -} - export default function daemon() { log('STARTING daemon'); WatchmanFileCache.initialize().then(() => { diff --git a/lib/log.js b/lib/log.js new file mode 100644 index 00000000..599da3b7 --- /dev/null +++ b/lib/log.js @@ -0,0 +1,11 @@ +import fs from 'fs'; + +export default function log(message) { + const timestamp = new Date().getTime(); + fs.appendFile( + '/tmp/importjsd.log', `${timestamp}: ${message}\n`, (err) => { + if (err) { + throw new Error(err); + } + }); +} From f6c28dc64c06bf23c1292346974b70f0a4a79b74 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Tue, 21 Jun 2016 00:04:05 +0200 Subject: [PATCH 15/41] Add some docs for `importjsd` The daemon is starting to stabilize a little, and I thought I'd document a little how it works. --- CONTRIBUTING.md | 16 ++++++++++++++++ README.md | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3774f570..a580cd72 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -39,6 +39,22 @@ Using Jest CLI v11.0.2, jasmine2, babel-jest 285 tests passed (285 total in 7 test suites, run time 29.123s) ``` +## Testing the daemon + +This is how you can test the `importjsd` daemon from the command line. + +```sh +mkfifo IMPORTJS +importjsd < IMPORTJS +``` + +The `importjs` process will now listen to `stdin` from the named pipe +(`"IMPORTJS"`). You can print to it using echo: + +```sh +echo '{"fileContent": "...", "pathToFile": "foo.js", "command": "fix"}' > IMPORTJS +``` + ## Publishing First ensure that your master is up to date: diff --git a/README.md b/README.md index 56abd997..6d93c120 100644 --- a/README.md +++ b/README.md @@ -477,6 +477,46 @@ Since the `--overwrite` flag makes ImportJS destructive (files are overwritten), it's a good thing to double-check that the `find` command returns the right files before adding the `-exec` part. +## Running as a daemon + +To speed up importing, import-js can run as a daemon. Start the daemon by +running `importjsd`. The daemon accepts commands sent via `stdin`. Each command +is a (oneline) JSON string ending with a newline. The command structure is +basically the same as for the command-line tool, but wrapped in JSON instead of +expressed on the command line. Here are a few examples: + +Run `fix imports`: +```json +{ + "command": "fix", + "fileContent": "const foo = bar();\n", + "pathToFile": "foo.js", +} +``` + +Import a single word: +```json +{ + "command": "word", + "commandArg": "bar", + "fileContent": "const foo = bar();\n", + "pathToFile": "foo.js", +} +``` + +Goto: +```json +{ + "command": "goto", + "commandArg": "bar", + "fileContent": "const foo = bar();\n", + "pathToFile": "foo.js", +} +``` + +The daemon will print results to `stdout` in JSON format. The response will +look the same as what the command-line tool produces. + ## Contributing See the [CONTRIBUTING.md](CONTRIBUTING.md) document for tips on how to run, test From 44148d7f5bc92bc29190979c9cbf9a39a7032b0c Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Tue, 21 Jun 2016 20:24:43 +0200 Subject: [PATCH 16/41] Make daemon work when watchman binary is unavailable During development, I haven't tested import-js without watchman being installed. I asked @lencioni to give it a spin without first installing watchman, and he reported seeing this error: Watchman: Watchman was not found in PATH. See https://facebook.github.io/watchman/docs/install.html for installation instructions events.js:154 throw er; // Unhandled 'error' event ^ Error: Watchman was not found in PATH. See https://facebook.github.io/watchman/docs/install.html for installation instructions at exports._errnoException (util.js:890:11) at Process.ChildProcess._handle.onexit (internal/child_process.js:182:32) at onErrorNT (internal/child_process.js:348:16) at _combinedTickCallback (node.js:377:13) at process._tickCallback (node.js:401:11) at Function.Module.runMain (module.js:449:11) at startup (node.js:141:18) at node.js:933:3 It turns out that we have to subscribe to the `error` event from the watchman Client to catch this error. With the changes in this commit, you are now able to run the daemon without having watchman installed (in which case we fallback to using a different file finding strategy). --- lib/WatchmanFileCache.js | 3 +++ lib/daemon.js | 2 +- lib/findMatchingFiles.js | 3 ++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/WatchmanFileCache.js b/lib/WatchmanFileCache.js index e4f01397..290e7289 100644 --- a/lib/WatchmanFileCache.js +++ b/lib/WatchmanFileCache.js @@ -82,6 +82,9 @@ export default { initialize(): Promise { return new Promise((resolve, reject) => { const client = new fbWatchman.Client(); + client.on('error', (error) => { + reject(error); + }); client.capabilityCheck({ optional: [], required: ['relative_root'], diff --git a/lib/daemon.js b/lib/daemon.js index e7998f0f..3c016da7 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -17,7 +17,7 @@ export default function daemon() { WatchmanFileCache.initialize().then(() => { log('WATCHMAN file cache is enabled'); }).catch((err: Object) => { - log(`WATCHMAN file cache is not available. Error: ${err.stack}`); + log(`WATCHMAN file cache is not available. Reason:\n${err.stack}`); }); const rlInterface = readline.createInterface({ diff --git a/lib/findMatchingFiles.js b/lib/findMatchingFiles.js index 5d7008aa..f66d64cb 100644 --- a/lib/findMatchingFiles.js +++ b/lib/findMatchingFiles.js @@ -4,8 +4,9 @@ import childProcess from 'child_process'; import glob from 'glob'; -import formattedToRegex from './formattedToRegex'; import WatchmanFileCache from './WatchmanFileCache'; +import formattedToRegex from './formattedToRegex'; +import normalizePath from './normalizePath'; function findMatchingFilesWithFind( lookupPath: string, From 85cef1303f018958d18de7880cc204bf0e10a0af Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Tue, 21 Jun 2016 20:31:00 +0200 Subject: [PATCH 17/41] Make watchman file finder honor the `excludes` config option The watchman file finder is starting to stabilize, and I really need to add tests for it. I found a bug while testing it out locally. The `excludes` config option is meant to filter out files, but that wasn't working for the new finder because we didn't normalize the lookup path the same way the file paths were. --- lib/findMatchingFiles.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/findMatchingFiles.js b/lib/findMatchingFiles.js index f66d64cb..caed252e 100644 --- a/lib/findMatchingFiles.js +++ b/lib/findMatchingFiles.js @@ -57,11 +57,12 @@ function findMatchingFilesWithWatchman( validFilesRegex: string ): Promise> { const tester = new RegExp(validFilesRegex, 'i'); + const normalizedLookupPath = normalizePath(lookupPath); return new Promise((resolve: Function) => { const matches = []; // `getFiles()` returns a Set, so we can't use `filter` here. WatchmanFileCache.getFiles().forEach((filePath: string) => { - if (!filePath.startsWith(lookupPath)) { + if (!filePath.startsWith(normalizedLookupPath)) { return; } if (tester.test(filePath)) { From 394d971253556671ec7bfbe474f31865b4873b1c Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Tue, 21 Jun 2016 20:42:56 +0200 Subject: [PATCH 18/41] Memoize cwd pattern in normalizePath @lencioni was reporting seeing ENFILE errors when running importjsd locally. I think they come from the fact that we issue a `process.cwd()` command every time we normalize a path. events.js:154 throw er; // Unhandled 'error' event ^ Error: ENFILE: file table overflow, uv_cwd at Error (native) at normalizePath (/Users/joe_lencioni/import-js/build/normalizePath.js:18:79) at /Users/joe_lencioni/import-js/build/WatchmanFileCache.js:61:58 at Array.forEach (native) at Client. (/Users/joe_lencioni/import-js/build/WatchmanFileCache.js:60:21) at emitOne (events.js:90:13) at Client.emit (events.js:182:7) at BunserBuf. (/Users/joe_lencioni/import-js/node_modules/fb-watchman/index.js:90:14) at emitOne (events.js:90:13) at BunserBuf.emit (events.js:182:7) Even though cwd might change during the lifetime of the process (outside of our control), I can't see why memoizing it would be bad. It might in fact help prevent subtle bugs. --- lib/normalizePath.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/normalizePath.js b/lib/normalizePath.js index 3ec96645..9aea6d4c 100644 --- a/lib/normalizePath.js +++ b/lib/normalizePath.js @@ -1,10 +1,12 @@ import escapeRegExp from 'lodash.escaperegexp'; +const CWD_PATTERN = RegExp(`^${escapeRegExp(process.cwd())}`); + export default function normalizePath(rawPath: string): string { if (!rawPath) { return './'; } - const normalized = rawPath.replace(RegExp(`^${escapeRegExp(process.cwd())}`), '.'); + const normalized = rawPath.replace(CWD_PATTERN, '.'); if (normalized.startsWith('.')) { return normalized; } From af4c3703c90c3170d7fcd45bcc691dd7fedd059f Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Tue, 21 Jun 2016 20:55:33 +0200 Subject: [PATCH 19/41] Temporarily disable logging I think this is causing issues for @lencioni. I plan on making logging a little more sophisticated. But before I do that, we can just temporarily disable it. --- lib/log.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/log.js b/lib/log.js index 599da3b7..77da5dc4 100644 --- a/lib/log.js +++ b/lib/log.js @@ -1,11 +1,11 @@ import fs from 'fs'; export default function log(message) { - const timestamp = new Date().getTime(); - fs.appendFile( - '/tmp/importjsd.log', `${timestamp}: ${message}\n`, (err) => { - if (err) { - throw new Error(err); - } - }); + // const timestamp = new Date().getTime(); + // fs.appendFile( + // '/tmp/importjsd.log', `${timestamp}: ${message}\n`, (err) => { + // if (err) { + // throw new Error(err); + // } + // }); } From b015212f5975cf60d89c570153f0ce4186c433ff Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Tue, 21 Jun 2016 23:40:29 +0200 Subject: [PATCH 20/41] Stop using deasync for findMatchingFiles test I was running in to some weird issues with deasync when adding the watchman-powered file finder to the mix. It wasn't doing much good, so I decided to ditch it in favor of making async assertions using the `done` callback. --- lib/__tests__/findMatchingFiles-test.js | 62 +++++++++++++++---------- 1 file changed, 37 insertions(+), 25 deletions(-) diff --git a/lib/__tests__/findMatchingFiles-test.js b/lib/__tests__/findMatchingFiles-test.js index 745acdd8..256faead 100644 --- a/lib/__tests__/findMatchingFiles-test.js +++ b/lib/__tests__/findMatchingFiles-test.js @@ -1,7 +1,6 @@ import fs from 'fs'; import path from 'path'; -import deasync from 'deasync'; import mkdirp from 'mkdirp'; import rimraf from 'rimraf'; @@ -44,12 +43,7 @@ import findMatchingFiles from '../findMatchingFiles'; fs.closeSync(fs.openSync(fullPath, 'w')); // create empty file }); - let result; - findMatchingFiles(lookupPath, word, finder).then((files) => { - result = files; - }); - deasync.loopWhile(() => !result); - return result; + return findMatchingFiles(lookupPath, word, finder); }; }); @@ -64,8 +58,11 @@ import findMatchingFiles from '../findMatchingFiles'; ]; }); - it('returns an empty array', () => { - expect(subject()).toEqual([]); + it('returns an empty array', (done) => { + subject().then((files) => { + expect(files).toEqual([]); + done(); + }); }); }); @@ -92,14 +89,17 @@ import findMatchingFiles from '../findMatchingFiles'; ]; }); - it('returns the matching files', () => { - expect(subject().sort()).toEqual([ - `${tmpDir}/bar/foo.js`, - `${tmpDir}/car/Foo.jsx`, - `${tmpDir}/har-har/foo/package.json`, - `${tmpDir}/react/foo/index.jsx`, - `${tmpDir}/tzar/foo/index.js`, - ]); + it('returns the matching files', (done) => { + subject().then((files) => { + expect(files.sort()).toEqual([ + `${tmpDir}/bar/foo.js`, + `${tmpDir}/car/Foo.jsx`, + `${tmpDir}/har-har/foo/package.json`, + `${tmpDir}/react/foo/index.jsx`, + `${tmpDir}/tzar/foo/index.js`, + ]); + done(); + }); }); }); @@ -108,24 +108,36 @@ import findMatchingFiles from '../findMatchingFiles'; word = 'FooBar'; }); - it('matches snake_case names', () => { + it('matches snake_case names', (done) => { existingFiles = ['foo_bar.js']; - expect(subject()).toEqual([`${tmpDir}/foo_bar.js`]); + subject().then((files) => { + expect(files).toEqual([`${tmpDir}/foo_bar.js`]); + done(); + }); }); - it('matches camelCase names', () => { + it('matches camelCase names', (done) => { existingFiles = ['fooBar.js']; - expect(subject()).toEqual([`${tmpDir}/fooBar.js`]); + subject().then((files) => { + expect(files).toEqual([`${tmpDir}/fooBar.js`]); + done(); + }); }); - it('matches folder + file name', () => { + it('matches folder + file name', (done) => { existingFiles = ['foo/Bar.js']; - expect(subject()).toEqual([`${tmpDir}/foo/Bar.js`]); + subject().then((files) => { + expect(files).toEqual([`${tmpDir}/foo/Bar.js`]); + done(); + }); }); - it('matches plural folder name + file name', () => { + it('matches plural folder name + file name', (done) => { existingFiles = ['foos/Bar.js']; - expect(subject()).toEqual([`${tmpDir}/foos/Bar.js`]); + subject().then((files) => { + expect(files).toEqual([`${tmpDir}/foos/Bar.js`]); + done(); + }); }); }); }); From 88071ecf784cb1e67fa4fc9cc941f473016d3ce3 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Wed, 22 Jun 2016 07:16:15 +0200 Subject: [PATCH 21/41] Add basic test for watchman file finder This first version simply mocks the `getFiles` function of WatchmanFileCache. We're not really testing too much here really, but at least we have a bit of test coverage for the conditional logic inside findMatchingFiles. --- lib/__tests__/findMatchingFiles-test.js | 50 ++++++++++++++++--------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/lib/__tests__/findMatchingFiles-test.js b/lib/__tests__/findMatchingFiles-test.js index 256faead..01dcf96a 100644 --- a/lib/__tests__/findMatchingFiles-test.js +++ b/lib/__tests__/findMatchingFiles-test.js @@ -1,12 +1,14 @@ +/* global spyOn */ import fs from 'fs'; import path from 'path'; import mkdirp from 'mkdirp'; import rimraf from 'rimraf'; +import WatchmanFileCache from '../WatchmanFileCache'; import findMatchingFiles from '../findMatchingFiles'; -['node', 'find'].forEach((finder) => { +['node', 'find', 'watchman'].forEach((finder) => { describe(`findMatchingFiles finder: ${finder}`, () => { const tmpDir = './tmp-findMatchingFiles'; let subject; @@ -30,22 +32,36 @@ import findMatchingFiles from '../findMatchingFiles'; }); } - beforeEach(() => { - fs.mkdirSync(tmpDir); - word = 'foo'; - existingFiles = []; - lookupPath = tmpDir; - - subject = () => { - existingFiles.forEach((file) => { - const fullPath = `${tmpDir}/${file}`; - mkdirp.sync(path.dirname(fullPath)); - fs.closeSync(fs.openSync(fullPath, 'w')); // create empty file - }); - - return findMatchingFiles(lookupPath, word, finder); - }; - }); + if (finder === 'watchman') { + beforeEach(() => { + word = 'foo'; + lookupPath = tmpDir; + + subject = () => { + spyOn(WatchmanFileCache, 'getFiles').and.returnValue( + new Set(existingFiles.map((file) => `${tmpDir}/${file}`))); + spyOn(WatchmanFileCache, 'isEnabled').and.returnValue(true); + return findMatchingFiles(lookupPath, word); + }; + }); + } else { + beforeEach(() => { + fs.mkdirSync(tmpDir); + word = 'foo'; + existingFiles = []; + lookupPath = tmpDir; + + subject = () => { + existingFiles.forEach((file) => { + const fullPath = `${tmpDir}/${file}`; + mkdirp.sync(path.dirname(fullPath)); + fs.closeSync(fs.openSync(fullPath, 'w')); // create empty file + }); + + return findMatchingFiles(lookupPath, word, finder); + }; + }); + } afterEach((done) => { rimraf(tmpDir, done); From 3356e1af32ef41f5d000347d6dabacce55f579b4 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Wed, 22 Jun 2016 07:35:35 +0200 Subject: [PATCH 22/41] Replace custom logging with loglevel package I want to get better control over logging. The previous hack (appending to file) was only meant to be a temporary solution. I considered Winston and Bunyan before deciding to go with the more lightweight loglevel [1]. Right now, we use `debug` level throughout. I'll make that configurable in a later commit. And I'll also pipe the output to a file (loglevel uses console internally). [1]: https://github.com/pimterry/loglevel --- lib/WatchmanFileCache.js | 8 ++++---- lib/daemon.js | 16 +++++++++------- lib/log.js | 11 ----------- package.json | 1 + 4 files changed, 14 insertions(+), 22 deletions(-) delete mode 100644 lib/log.js diff --git a/lib/WatchmanFileCache.js b/lib/WatchmanFileCache.js index 290e7289..e76b2413 100644 --- a/lib/WatchmanFileCache.js +++ b/lib/WatchmanFileCache.js @@ -1,6 +1,6 @@ import fbWatchman from 'fb-watchman'; +import loglevel from 'loglevel'; -import log from './log'; import normalizePath from './normalizePath'; const SUBSCRIPTION_NAME = 'import-js-subscription'; @@ -41,10 +41,10 @@ function subscribe({ client, fbWatch, relativePath }): Promise { filteredFiles.forEach((file) => { const normalizedPath = normalizePath(file.name); if (file.exists) { - log(`ADDING file to WatchmanFileCache: ${normalizedPath}`); + loglevel.debug(`ADDING file to WatchmanFileCache: ${normalizedPath}`); CACHE.add(normalizedPath); } else { - log(`REMOVING file from WatchmanFileCache: ${normalizedPath}`); + loglevel.debug(`REMOVING file from WatchmanFileCache: ${normalizedPath}`); CACHE.delete(normalizedPath); } }); @@ -61,7 +61,7 @@ function startSubscription({ client }): Promise { } if ('warning' in resp) { - log(`WARNING received during watchman init: ${resp.warning}`); + loglevel.warn(`WARNING received during watchman init: ${resp.warning}`); } subscribe({ diff --git a/lib/daemon.js b/lib/daemon.js index 3c016da7..14e16fef 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -1,8 +1,9 @@ import readline from 'readline'; +import loglevel from 'loglevel'; + import Importer from './Importer'; import WatchmanFileCache from './WatchmanFileCache'; -import log from './log'; const commandsToFunctionNames = { fix: 'fixImports', @@ -13,11 +14,12 @@ const commandsToFunctionNames = { }; export default function daemon() { - log('STARTING daemon'); + loglevel.setLevel('debug'); + loglevel.info('STARTING daemon'); WatchmanFileCache.initialize().then(() => { - log('WATCHMAN file cache is enabled'); + loglevel.info('WATCHMAN file cache is enabled'); }).catch((err: Object) => { - log(`WATCHMAN file cache is not available. Reason:\n${err.stack}`); + loglevel.info(`WATCHMAN file cache is not available. Reason:\n${err.stack}`); }); const rlInterface = readline.createInterface({ @@ -27,7 +29,7 @@ export default function daemon() { }); rlInterface.on('line', (jsonPayload: string) => { - log(`RECEIVED payload: ${jsonPayload}`); + loglevel.debug(`RECEIVED payload: ${jsonPayload}`); // The json payload is an array containing a job id and job details. const payload = JSON.parse(jsonPayload); const importer = new Importer( @@ -37,13 +39,13 @@ export default function daemon() { const functionName = commandsToFunctionNames[payload.command]; importer[functionName](payload.commandArg).then((result: Object) => { const jsonResponse = JSON.stringify(result); - log(`SENDING response: ${jsonResponse}`); + loglevel.debug(`SENDING response: ${jsonResponse}`); process.stdout.write(`${jsonResponse}\n`); }).catch((error: Object) => { const jsonResponse = JSON.stringify({ error: error.stack, }); - log(`ERROR response: ${jsonResponse}`); + loglevel.error(`ERROR response: ${jsonResponse}`); process.stdout.write(`${jsonResponse}\n`); }); }); diff --git a/lib/log.js b/lib/log.js deleted file mode 100644 index 77da5dc4..00000000 --- a/lib/log.js +++ /dev/null @@ -1,11 +0,0 @@ -import fs from 'fs'; - -export default function log(message) { - // const timestamp = new Date().getTime(); - // fs.appendFile( - // '/tmp/importjsd.log', `${timestamp}: ${message}\n`, (err) => { - // if (err) { - // throw new Error(err); - // } - // }); -} diff --git a/package.json b/package.json index a5b8e58c..21648254 100644 --- a/package.json +++ b/package.json @@ -71,6 +71,7 @@ "lodash.partition": "^4.2.0", "lodash.sortby": "^4.2.2", "lodash.uniqby": "^4.2.1", + "loglevel": "^1.4.1", "minimatch": "^3.0.0", "semver": "^5.1.0", "xregexp": "^3.1.1" From 2dc16c1e2e35082d96c07a7c650f6ab8dfff17f6 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Wed, 22 Jun 2016 08:15:29 +0200 Subject: [PATCH 23/41] Redirect console logging to file We need to be careful with what we're logging to stdout, because clients will be depending on json content in a certain format ending up there. We log to a file called `importjs.log` in the os's default tmp directory. Since this folder isn't super easy to discover, I'll follow up by logging the path to the logfile on startup. --- lib/daemon.js | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/lib/daemon.js b/lib/daemon.js index 14e16fef..a3c75ab4 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -1,10 +1,30 @@ +import fs from 'fs'; +import os from 'os'; +import path from 'path'; import readline from 'readline'; +import util from 'util'; import loglevel from 'loglevel'; +import Configuration from './Configuration'; import Importer from './Importer'; import WatchmanFileCache from './WatchmanFileCache'; +const logFile = fs.createWriteStream(path.join(os.tmpdir(), 'importjs.log'), { + flags: 'w', +}); + +/* eslint-disable no-console */ +console.log = (...args) => { + logFile.write(`${util.format.apply(null, args)}\n`); +}; +console.trace = console.log; +console.debug = console.log; +console.info = console.log; +console.warn = console.log; +console.error = console.log; +/* eslint-enable no-console */ + const commandsToFunctionNames = { fix: 'fixImports', word: 'import', From 6ff3e74eb1eb2e231c8a2d14461a9b4b51f2de93 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Wed, 22 Jun 2016 08:20:47 +0200 Subject: [PATCH 24/41] Make logLevel configurable I'm starting to think that we need to support user-specific configuration. Having the loglevel specified in importjs.json is problematic, since you usually share this file with others. For now though, I think it's fine to keep it here. --- .importjs.json | 1 + lib/Configuration.js | 2 ++ lib/daemon.js | 6 +++++- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/.importjs.json b/.importjs.json index 227be736..59a25a8c 100644 --- a/.importjs.json +++ b/.importjs.json @@ -2,6 +2,7 @@ { "environments": ["node"], "ignorePackagePrefixes": ["lodash."], + "logLevel": "debug", "excludes": [ "./build/**", "./lib/__mocks__/**" diff --git a/lib/Configuration.js b/lib/Configuration.js index 0524279d..85043f9c 100644 --- a/lib/Configuration.js +++ b/lib/Configuration.js @@ -23,6 +23,7 @@ const DEFAULT_CONFIG = { ignorePackagePrefixes: [], importDevDependencies: false, importFunction: 'require', + logLevel: 'info', lookupPaths: ['.'], maxLineLength: 80, minimumVersion: '0.0.0', @@ -105,6 +106,7 @@ const KNOWN_CONFIGURATION_OPTIONS = [ 'ignorePackagePrefixes', 'importDevDependencies', 'importFunction', + 'logLevel', 'lookupPaths', 'maxLineLength', 'minimumVersion', diff --git a/lib/daemon.js b/lib/daemon.js index a3c75ab4..67a02dcf 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -34,7 +34,11 @@ const commandsToFunctionNames = { }; export default function daemon() { - loglevel.setLevel('debug'); + // The `importjsd` here is mostly a dummy file because config relies on a + // `pathToCurrentFile`. Normally, this is the javascript file you are editing. + const config = new Configuration('importjsd'); + + loglevel.setLevel(config.get('logLevel')); loglevel.info('STARTING daemon'); WatchmanFileCache.initialize().then(() => { loglevel.info('WATCHMAN file cache is enabled'); From cb1eca0a4306abe9f49e4825ea567c0cf7e53a6b Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Wed, 22 Jun 2016 08:28:29 +0200 Subject: [PATCH 25/41] Print path to logfile on daemon startup This will help people who want to find out what's going on behind the scenes. --- lib/daemon.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/daemon.js b/lib/daemon.js index 67a02dcf..31b05a34 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -10,13 +10,15 @@ import Configuration from './Configuration'; import Importer from './Importer'; import WatchmanFileCache from './WatchmanFileCache'; -const logFile = fs.createWriteStream(path.join(os.tmpdir(), 'importjs.log'), { +const pathToLogFile = path.join(os.tmpdir(), 'importjs.log'); +const logStream = fs.createWriteStream(pathToLogFile, { flags: 'w', }); /* eslint-disable no-console */ +const originalConsoleLog = console.log; console.log = (...args) => { - logFile.write(`${util.format.apply(null, args)}\n`); + logStream.write(`${util.format.apply(null, args)}\n`); }; console.trace = console.log; console.debug = console.log; @@ -39,7 +41,8 @@ export default function daemon() { const config = new Configuration('importjsd'); loglevel.setLevel(config.get('logLevel')); - loglevel.info('STARTING daemon'); + originalConsoleLog(`DAEMON active. Logs will go to: \n${pathToLogFile}`); + WatchmanFileCache.initialize().then(() => { loglevel.info('WATCHMAN file cache is enabled'); }).catch((err: Object) => { From 5828579aef5b0a4119fa9aaff4c035b3c0d4e535 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Wed, 22 Jun 2016 08:34:15 +0200 Subject: [PATCH 26/41] Add info about logging to README When importjs is run as a daemon, logging will be important so that possible issues can be detected. We want people to be able to control how much they see of this. --- README.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/README.md b/README.md index 6d93c120..860a0e30 100644 --- a/README.md +++ b/README.md @@ -117,6 +117,7 @@ configure ImportJS. The following configuration options can be used. - [`minimumVersion`](#minimumversion) - [`maxLineLength`](#maxlinelength) - [`tab`](#tab) +- [`logLevel`](#loglevel) ### `lookupPaths` @@ -380,6 +381,19 @@ constructed when import statements are broken into multiple lines. "tab": "\t" ``` +### `logLevel` + +One of `["debug", "info", "warn", "error"]`. This controls what ends up in the +logfile (mostly used when [import-js is run as a daemon +process](#running-as-a-daemon). The default is `info`. + +```json +"logLevel": "debug" +``` + +*Tip:* Don't put `node_modules` here. ImportJS will find your Node dependencies +through your `package.json` file. + ## Local configuration You can dynamically apply configuration to different directory trees within your @@ -517,6 +531,12 @@ Goto: The daemon will print results to `stdout` in JSON format. The response will look the same as what the command-line tool produces. +On startup, the daemon will print a path to a logfile. If you want to find out +what's going on behind the scenes, you can inspect this file. If you don't have +access to the console log of the daemon, you'll find the logfile in +`os.tmpdir() + '/importjs.log` (which will resolve to something like +`var/folders/1l/_t6tm7195nd53936tsvh2pcr0000gn/T/importjs.log` on a Mac). + ## Contributing See the [CONTRIBUTING.md](CONTRIBUTING.md) document for tips on how to run, test From cdb95884387e9dbc5812180ab29a23d98fd88f70 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Wed, 22 Jun 2016 08:45:54 +0200 Subject: [PATCH 27/41] Prefix all logs with timestamp and level The loglevel-message-prefix [1] takes care of that. [1]: https://github.com/NatLibFi/loglevel-message-prefix --- lib/daemon.js | 2 ++ package.json | 1 + 2 files changed, 3 insertions(+) diff --git a/lib/daemon.js b/lib/daemon.js index 31b05a34..0e3b18a4 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -5,6 +5,7 @@ import readline from 'readline'; import util from 'util'; import loglevel from 'loglevel'; +import loglevelMessagePrefix from 'loglevel-message-prefix'; import Configuration from './Configuration'; import Importer from './Importer'; @@ -41,6 +42,7 @@ export default function daemon() { const config = new Configuration('importjsd'); loglevel.setLevel(config.get('logLevel')); + loglevelMessagePrefix(loglevel); originalConsoleLog(`DAEMON active. Logs will go to: \n${pathToLogFile}`); WatchmanFileCache.initialize().then(() => { diff --git a/package.json b/package.json index 21648254..684d0f09 100644 --- a/package.json +++ b/package.json @@ -72,6 +72,7 @@ "lodash.sortby": "^4.2.2", "lodash.uniqby": "^4.2.1", "loglevel": "^1.4.1", + "loglevel-message-prefix": "^1.1.1", "minimatch": "^3.0.0", "semver": "^5.1.0", "xregexp": "^3.1.1" From ff2474b6553f4bbf396f824021a4052fb6672742 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Wed, 22 Jun 2016 09:11:40 +0200 Subject: [PATCH 28/41] Add info about Watchman to README This bit of information should also be added to the README files of the plugin (once they get updated to use the daemon). --- README.md | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 860a0e30..74ac4d42 100644 --- a/README.md +++ b/README.md @@ -95,6 +95,9 @@ the cursor on a variable and hit `g` (Vim), `(M-x) import-js-goto` - As part of resolving imports, all imports will be sorted and placed into groups. *Grouping can be disabled, see the `groupImports` configuration option.* +- You can speed up importing by installing + (Watchman)[https://facebook.github.io/watchman/]. See [Speeding it + up!](#speeding-it-up) for more information. ## Configuration @@ -384,7 +387,7 @@ constructed when import statements are broken into multiple lines. ### `logLevel` One of `["debug", "info", "warn", "error"]`. This controls what ends up in the -logfile (mostly used when [import-js is run as a daemon +logfile (mostly used when [ImportJS is run as a daemon process](#running-as-a-daemon). The default is `info`. ```json @@ -493,11 +496,17 @@ files before adding the `-exec` part. ## Running as a daemon -To speed up importing, import-js can run as a daemon. Start the daemon by -running `importjsd`. The daemon accepts commands sent via `stdin`. Each command -is a (oneline) JSON string ending with a newline. The command structure is -basically the same as for the command-line tool, but wrapped in JSON instead of -expressed on the command line. Here are a few examples: +Instead of invoking the `importjs` command-line tool every time you import +something, you can run ImportJS in a background process and communicate with +it using `stdin` and `stdout`. This will make importing faster because we don't +have to spin up a node environment on every invocation. Chances are your editor +plugin is already using the daemon for importing. + +The daemon is started by running running `importjsd`. It accepts commands sent +via `stdin`. Each command is a (oneline) JSON string ending with a newline. The +command structure is basically the same as for the command-line tool, but +wrapped in JSON instead of expressed on the command line. Here are a few +examples: Run `fix imports`: ```json @@ -537,6 +546,17 @@ access to the console log of the daemon, you'll find the logfile in `os.tmpdir() + '/importjs.log` (which will resolve to something like `var/folders/1l/_t6tm7195nd53936tsvh2pcr0000gn/T/importjs.log` on a Mac). +## Speeding it up! + +If you have a large application, traversing the file system to find modules can +be slow. That's why ImportJS has built-in integration with +[Watchman](https://facebook.github.io/watchman/), a fast and robust file +watching service developed by Facebook. All you have to do to get a performance +boost is to [install watchman +locally](https://facebook.github.io/watchman/docs/install.html), and make sure +to use an up-to-date editor plugin (Watchman is only used when ImportJS is run +as a daemon). + ## Contributing See the [CONTRIBUTING.md](CONTRIBUTING.md) document for tips on how to run, test From 9b6a7ad3bb1bf6eab69e153d6f88355f403a2bcd Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Sat, 25 Jun 2016 14:23:52 +0200 Subject: [PATCH 29/41] Exit daemon if dead parent detected By having all editor plugins pass in their process pids, we can make sure that the importjsd process never turns into a zombie. There's a chance that the original parent has died, but a new process has taken over the PID of the old parent. In that case, the importjsd process will become a zombie, even with the code included in this change. That's a somewhat unlikely scenario though, that I don't think we need to bother much about. According to [1], PIDs rotate (up to 32768) on linux systems so there would have to be a lot of processes started before the old PID is reused. Also included in this commit is changing the first log of the command to be one line. This is important for editors so that they know when they can start expecting JSON output on the channel. [1]: http://stackoverflow.com/questions/11323410/linux-pid-recycling --- lib/daemon.js | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/daemon.js b/lib/daemon.js index 0e3b18a4..6c3713c4 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -4,6 +4,7 @@ import path from 'path'; import readline from 'readline'; import util from 'util'; +import commander from 'commander'; import loglevel from 'loglevel'; import loglevelMessagePrefix from 'loglevel-message-prefix'; @@ -36,6 +37,8 @@ const commandsToFunctionNames = { add: 'addImports', }; +commander.option('--parent-pid ', parseInt).parse(process.argv); + export default function daemon() { // The `importjsd` here is mostly a dummy file because config relies on a // `pathToCurrentFile`. Normally, this is the javascript file you are editing. @@ -43,7 +46,7 @@ export default function daemon() { loglevel.setLevel(config.get('logLevel')); loglevelMessagePrefix(loglevel); - originalConsoleLog(`DAEMON active. Logs will go to: \n${pathToLogFile}`); + originalConsoleLog(`DAEMON active. Logs will go to: ${pathToLogFile}`); WatchmanFileCache.initialize().then(() => { loglevel.info('WATCHMAN file cache is enabled'); @@ -51,6 +54,21 @@ export default function daemon() { loglevel.info(`WATCHMAN file cache is not available. Reason:\n${err.stack}`); }); + if (commander.parentPid) { + // Editor plugins should provide a `--parent-pid=` argument on startup, + // so that we can check that the daemon process hasn't turned into a zombie + // once in a while. + setInterval(() => { + loglevel.debug(`Making sure that the parent process (PID=${commander.parentPid}) is still running.`); + try { + process.kill(commander.parentPid, 0); + } catch (error) { + loglevel.info('Parent process seems to have died. Exiting.'); + process.exit(1); + } + }, 30000); + } + const rlInterface = readline.createInterface({ input: process.stdin, output: process.stdout, From f1c8181599ceed7d8db51209178222ed3d57c505 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Sat, 25 Jun 2016 15:22:59 +0200 Subject: [PATCH 30/41] Fix eslint error in daemon.js --- lib/daemon.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/daemon.js b/lib/daemon.js index 6c3713c4..86cea3f8 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -59,7 +59,8 @@ export default function daemon() { // so that we can check that the daemon process hasn't turned into a zombie // once in a while. setInterval(() => { - loglevel.debug(`Making sure that the parent process (PID=${commander.parentPid}) is still running.`); + loglevel.debug('Making sure that the parent process ' + + `(PID=${commander.parentPid}) is still running.`); try { process.kill(commander.parentPid, 0); } catch (error) { From 7c2fcc0adf9848c9794f7e76a17fa7898cf0f445 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Sat, 25 Jun 2016 17:09:10 +0200 Subject: [PATCH 31/41] Add simple benchmark script Now that we've made some performance improvements, I want to make sure that we don't regress too much. A benchmark test can help us better surface how import-js performs under some circumstances. To begin with, I'm just testing the case of importing a single word. --- .importjs.json | 4 +++ lib/benchmark.js | 76 ++++++++++++++++++++++++++++++++++++++++++++++++ package.json | 1 + 3 files changed, 81 insertions(+) create mode 100644 lib/benchmark.js diff --git a/.importjs.json b/.importjs.json index 59a25a8c..1c5a209e 100644 --- a/.importjs.json +++ b/.importjs.json @@ -11,5 +11,9 @@ { "appliesTo": "lib/__tests__/**", "importDevDependencies": true + }, + { + "appliesTo": "lib/benchmark.js", + "importDevDependencies": true } ] diff --git a/lib/benchmark.js b/lib/benchmark.js new file mode 100644 index 00000000..620f4312 --- /dev/null +++ b/lib/benchmark.js @@ -0,0 +1,76 @@ +/* eslint-disable no-console */ +import fs from 'fs'; +import path from 'path'; + +import mkdirp from 'mkdirp'; +import rimraf from 'rimraf'; + +import Importer from './Importer'; +import WatchmanFileCache from './WatchmanFileCache'; + +const numberOfFiles = parseInt(process.argv[2], 10); +const numberOfFolders = numberOfFiles / 10; + +const tmpFolder = './benchmark-files'; +const cycles = 10; + +rimraf.sync(tmpFolder); + +console.log(`Setting up benchmark for ${numberOfFiles} files, ` + + `in ${numberOfFolders} folders`); + +for (let i = 0; i < numberOfFiles; i++) { + const folderNumber = i % numberOfFolders; + const fullPath = `${tmpFolder}/folder-${folderNumber}/file${i}.js`; + mkdirp.sync(path.dirname(fullPath)); + fs.closeSync(fs.openSync(fullPath, 'w')); // create empty file +} + +function runImportIter(counter, done) { + const importer = new Importer( + [ + 'file1();', + ], + 'dummy.js' + ); + + importer.import('file1').then((result) => { + if (!/^import file1 from/.test(result.fileContent)) { + // We expect something to be imported. If not, something is wrong + throw result; + } + if (counter === cycles) { + done(); + } else { + runImportIter(counter + 1, done); + } + }).catch((error) => { + console.error(error); + }); +} + +function runTest(done) { + const startTime = new Date().getTime(); + runImportIter(1, () => { + const stopTime = new Date().getTime(); + const diff = stopTime - startTime; + const timePerCycle = diff / cycles; + console.log(`Total time ${diff}ms`); + console.log(`Time per cycle ${timePerCycle}ms`); + done(); + }); +} + +console.log('Testing (without watchman)...'); +runTest(() => { + console.log('Testing (with watchman)...'); + WatchmanFileCache.initialize().then(() => { + runTest(() => { + console.log('All done.'); + process.exit(); + }); + }).catch((error) => { + console.error(error); + process.exit(); + }); +}); diff --git a/package.json b/package.json index 684d0f09..bb7acc48 100644 --- a/package.json +++ b/package.json @@ -8,6 +8,7 @@ }, "main": "build/Importer.js", "scripts": { + "benchmark": "node build/benchmark.js 1000", "build": "babel lib --out-dir build", "clean": "rimraf build", "flow": "flow; test $? -eq 0 -o $? -eq 2", From 138991681507637919d3494c53b99236d60a9ee3 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Sat, 25 Jun 2016 17:14:08 +0200 Subject: [PATCH 32/41] Make benchmark script use `fix` command instead I use `fix` a lot more than importing a single word. I want the benchmark script to be more similar to what import-js does in the wild, so I'm switching to `fix`. This is good because it also tests the parallel file finding stuff that we do. --- lib/benchmark.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/benchmark.js b/lib/benchmark.js index 620f4312..2ccd87f7 100644 --- a/lib/benchmark.js +++ b/lib/benchmark.js @@ -30,11 +30,17 @@ function runImportIter(counter, done) { const importer = new Importer( [ 'file1();', + 'file2();', + 'file3();', + 'file4();', + 'file5();', + 'file6();', + 'file7();', ], 'dummy.js' ); - importer.import('file1').then((result) => { + importer.fixImports().then((result) => { if (!/^import file1 from/.test(result.fileContent)) { // We expect something to be imported. If not, something is wrong throw result; From ba432d6c58d89f9c1754175226eb43091731b923 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Sun, 26 Jun 2016 21:09:11 +0200 Subject: [PATCH 33/41] Kill blank line in fix-node-path.js --- lib/fix-node-path.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/fix-node-path.js b/lib/fix-node-path.js index 69d462e1..a222bbd6 100644 --- a/lib/fix-node-path.js +++ b/lib/fix-node-path.js @@ -11,4 +11,3 @@ const oldPath = process.env.NODE_PATH; process.env.NODE_PATH = `${oldPath || ''}:${process.cwd()}/node_modules/`; require('module').Module._initPaths(); // eslint-disable-line no-underscore-dangle - From 9424d7ccb8b3fd99d2b62be6854b67ed663af217 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Sun, 26 Jun 2016 23:14:11 +0200 Subject: [PATCH 34/41] Write error response if command is unknown Instead of failing with something like "undefined is not a function", we can print a friendly error message if an unknown command is being used. --- lib/daemon.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/daemon.js b/lib/daemon.js index 86cea3f8..31374c51 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -83,8 +83,17 @@ export default function daemon() { const importer = new Importer( payload.fileContent.split('\n'), payload.pathToFile); - // TODO: don't automatically invoke function here because of security issues const functionName = commandsToFunctionNames[payload.command]; + if (!functionName) { + const errorString = + `Unknown command: ${payload.command}. ` + + `Valid ones are ${Object.keys(commandsToFunctionNames).join(', ')}`; + loglevel.error(errorString); + const jsonResponse = JSON.stringify({ error: errorString }); + process.stdout.write(`${jsonResponse}\n`); + return; + } + importer[functionName](payload.commandArg).then((result: Object) => { const jsonResponse = JSON.stringify(result); loglevel.debug(`SENDING response: ${jsonResponse}`); From d061f98c4b5d4d38458f06a37d7c226a76a26e45 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Mon, 27 Jun 2016 21:07:55 +0200 Subject: [PATCH 35/41] Switch to `suffix` matching for watchman listener This allows us to get a little better control over what files are added to the cache. The only difference I noticed from before is that I no longer get flashes of ADDING file foo.js~ REMOVING file foo.js~ from the log. Which is nice. --- lib/WatchmanFileCache.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/WatchmanFileCache.js b/lib/WatchmanFileCache.js index e76b2413..f267a601 100644 --- a/lib/WatchmanFileCache.js +++ b/lib/WatchmanFileCache.js @@ -12,8 +12,13 @@ let enabled = false; function subscribe({ client, fbWatch, relativePath }): Promise { const subscription = { - // Match any `.js` file - expression: ['allof', ['match', '*.js*']], + // Match javascript files + expression: [ + 'anyof', + ['suffix', 'js'], + ['suffix', 'jsx'], + ['suffix', 'json'], + ], // We're only interested in the file name fields: ['name', 'exists'], relative_root: relativePath, From e404b286f7ee3f9bc1cd4230bf162069a7bbe1aa Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Mon, 27 Jun 2016 21:41:43 +0200 Subject: [PATCH 36/41] Rename regexp variables in findMatchingFiles.js There was some confusion around these variables, so I made it clearer what is a compiled regexp and what is just a string pattern. Before making the rename, I went down the path of switching to passing in a compiled RegExp to all places. But I couldn't get the `egrep` call inside `findMatchingFilesWithFind` to work with the `toString()` output of RegExp. So I decided to keep the string throughout all finders (so that it's consistent). --- lib/findMatchingFiles.js | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/lib/findMatchingFiles.js b/lib/findMatchingFiles.js index caed252e..b8b36cc6 100644 --- a/lib/findMatchingFiles.js +++ b/lib/findMatchingFiles.js @@ -10,14 +10,14 @@ import normalizePath from './normalizePath'; function findMatchingFilesWithFind( lookupPath: string, - validFilesRegex: string + validFilesPattern: string ): Promise> { const findCommand = [ `find ${lookupPath}`, '-name "**.js*"', '-not -path "./node_modules/*"', ].join(' '); - const command = `${findCommand} | egrep -i \"${validFilesRegex}\"`; + const command = `${findCommand} | egrep -i \"${validFilesPattern}\"`; return new Promise((resolve: Function, reject: Function) => { // TODO switch to spawn so we can start processing the stream as it comes @@ -36,8 +36,9 @@ function findMatchingFilesWithFind( function findMatchingFilesWithNode( lookupPath: string, - validFilesRegex: string + validFilesPattern: string ): Promise> { + const validFilesRegex = new RegExp(validFilesPattern, 'i'); return new Promise((resolve: Function, reject: Function) => { glob(`${lookupPath}/**/*.js*`, { ignore: './node_modules/**', @@ -47,16 +48,16 @@ function findMatchingFilesWithNode( return; } resolve(result.filter((filePath: string): bool => - new RegExp(validFilesRegex, 'i').test(filePath))); + validFilesRegex.test(filePath))); }); }); } function findMatchingFilesWithWatchman( lookupPath: string, - validFilesRegex: string + validFilesPattern: string ): Promise> { - const tester = new RegExp(validFilesRegex, 'i'); + const validFilesRegex = new RegExp(validFilesPattern, 'i'); const normalizedLookupPath = normalizePath(lookupPath); return new Promise((resolve: Function) => { const matches = []; @@ -65,7 +66,7 @@ function findMatchingFilesWithWatchman( if (!filePath.startsWith(normalizedLookupPath)) { return; } - if (tester.test(filePath)) { + if (validFilesRegex.test(filePath)) { matches.push(filePath); } }); @@ -87,14 +88,14 @@ export default function findMatchingFiles( } const formattedVarName = formattedToRegex(variableName); - const validFilesRegex = `(/|^)${formattedVarName}(/index)?(/package)?\\.js.*`; + const validFilesPattern = `(/|^)${formattedVarName}(/index)?(/package)?\\.js.*`; if (WatchmanFileCache.isEnabled()) { - return findMatchingFilesWithWatchman(lookupPath, validFilesRegex); + return findMatchingFilesWithWatchman(lookupPath, validFilesPattern); } if (/^win/.test(process.platform) || process.env.IMPORT_JS_USE_NODE_FINDER) { - return findMatchingFilesWithNode(lookupPath, validFilesRegex); + return findMatchingFilesWithNode(lookupPath, validFilesPattern); } - return findMatchingFilesWithFind(lookupPath, validFilesRegex); + return findMatchingFilesWithFind(lookupPath, validFilesPattern); } From 07ecfa35343b86455f917a760eb3a7a67bf0b62a Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Tue, 28 Jun 2016 21:29:36 +0200 Subject: [PATCH 37/41] Clarify section about daemon in README @lencioni pointed out in code review that we could be more clear about when you need to know about the daemon. I added a note about it being mostly useful for developers of editor plugins. And I changed the wording for a few other sub sections as well. --- README.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 74ac4d42..828284df 100644 --- a/README.md +++ b/README.md @@ -496,11 +496,13 @@ files before adding the `-exec` part. ## Running as a daemon -Instead of invoking the `importjs` command-line tool every time you import -something, you can run ImportJS in a background process and communicate with -it using `stdin` and `stdout`. This will make importing faster because we don't -have to spin up a node environment on every invocation. Chances are your editor -plugin is already using the daemon for importing. +*Note*: This section is intended mostly for developers of editor plugins. If +you are using one of the standard editor plugins, you are most likely using the +daemon under the hood already. + +You can run ImportJS in a background process and communicate with it using +`stdin` and `stdout`. This will make importing faster because we're not +spinning up a node environment on every invocation. The daemon is started by running running `importjsd`. It accepts commands sent via `stdin`. Each command is a (oneline) JSON string ending with a newline. The @@ -537,8 +539,9 @@ Goto: } ``` -The daemon will print results to `stdout` in JSON format. The response will -look the same as what the command-line tool produces. +Results are printed to `stdout` in JSON format. The response will look the same +as what the command-line tool produces. If an error occurs, it will also end up +in `stdout` as JSON (an object with an `error` key). On startup, the daemon will print a path to a logfile. If you want to find out what's going on behind the scenes, you can inspect this file. If you don't have From 3dc3e76215f6072272a922edcf0bf356d5dde1b0 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Tue, 28 Jun 2016 21:37:43 +0200 Subject: [PATCH 38/41] Move `console.log` rerouting into own module This makes the daemon.js file a little cleaner. As suggested by @lencioni in code review. --- lib/daemon.js | 19 ++----------------- lib/rerouteConsoleLog.js | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 17 deletions(-) create mode 100644 lib/rerouteConsoleLog.js diff --git a/lib/daemon.js b/lib/daemon.js index 31374c51..c776d60d 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -1,8 +1,6 @@ -import fs from 'fs'; import os from 'os'; import path from 'path'; import readline from 'readline'; -import util from 'util'; import commander from 'commander'; import loglevel from 'loglevel'; @@ -11,23 +9,10 @@ import loglevelMessagePrefix from 'loglevel-message-prefix'; import Configuration from './Configuration'; import Importer from './Importer'; import WatchmanFileCache from './WatchmanFileCache'; +import rerouteConsoleLog from './rerouteConsoleLog'; const pathToLogFile = path.join(os.tmpdir(), 'importjs.log'); -const logStream = fs.createWriteStream(pathToLogFile, { - flags: 'w', -}); - -/* eslint-disable no-console */ -const originalConsoleLog = console.log; -console.log = (...args) => { - logStream.write(`${util.format.apply(null, args)}\n`); -}; -console.trace = console.log; -console.debug = console.log; -console.info = console.log; -console.warn = console.log; -console.error = console.log; -/* eslint-enable no-console */ +const originalConsoleLog = rerouteConsoleLog(pathToLogFile); const commandsToFunctionNames = { fix: 'fixImports', diff --git a/lib/rerouteConsoleLog.js b/lib/rerouteConsoleLog.js new file mode 100644 index 00000000..c7afb276 --- /dev/null +++ b/lib/rerouteConsoleLog.js @@ -0,0 +1,24 @@ +import fs from 'fs'; +import util from 'util'; + +/** + * Reroutes console logs to a file. Returns the original `console.log`. + */ +export default function rerouteConsoleLog(pathToLogFile: string): Function { + const logStream = fs.createWriteStream(pathToLogFile, { + flags: 'w', + }); + + /* eslint-disable no-console */ + const originalConsoleLog = console.log; + console.log = (...args) => { + logStream.write(`${util.format.apply(null, args)}\n`); + }; + console.trace = console.log; + console.debug = console.log; + console.info = console.log; + console.warn = console.log; + console.error = console.log; + /* eslint-enable no-console */ + return originalConsoleLog; +} From 00d3295e9fa53f396401e276643722095cd3ebc5 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Tue, 28 Jun 2016 21:45:01 +0200 Subject: [PATCH 39/41] Make config for logLevelMessagePrefix explicit Even though this is the default, having the prefixes listed will make the code less magic, and more straightforward. https://github.com/NatLibFi/loglevel-message-prefix#configuration --- lib/daemon.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/daemon.js b/lib/daemon.js index c776d60d..5abd874a 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -30,7 +30,9 @@ export default function daemon() { const config = new Configuration('importjsd'); loglevel.setLevel(config.get('logLevel')); - loglevelMessagePrefix(loglevel); + loglevelMessagePrefix(loglevel, { + prefixes: ['timestamp', 'level'], + }); originalConsoleLog(`DAEMON active. Logs will go to: ${pathToLogFile}`); WatchmanFileCache.initialize().then(() => { From 8a75d50d7fabda000e4313c9f364979215215705 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Tue, 28 Jun 2016 21:48:40 +0200 Subject: [PATCH 40/41] Add PID of parent process to log messages Since all processes of importjsd will log to the same file, keeping logs separated can be a little tricky. By adding the parentPid to the mix, you can follow logs for a specific session. E.g.: tail -f /var/folders/1l/_t6tm7195n00gn/T/importjs.log | grep PID:12345 As suggested by @lencioni: https://github.com/Galooshi/import-js/pull/287/files/ff2474b6553f4bbf396f824021a4052fb6672742..9b6a7ad3bb1bf6eab69e153d6f88355f403a2bcd#r68489982 --- lib/daemon.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/daemon.js b/lib/daemon.js index 5abd874a..45b52989 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -32,6 +32,7 @@ export default function daemon() { loglevel.setLevel(config.get('logLevel')); loglevelMessagePrefix(loglevel, { prefixes: ['timestamp', 'level'], + staticPrefixes: [`PID:${commander.parentPid}`], }); originalConsoleLog(`DAEMON active. Logs will go to: ${pathToLogFile}`); From a803e9cd15f0ef398a2ad19532d9e0a6f0c6b028 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Tue, 28 Jun 2016 21:54:41 +0200 Subject: [PATCH 41/41] Add version info to daemon startup message This can come in handy when debugging for instance. As suggested by @lencioni: https://github.com/Galooshi/import-js/pull/287/files/ff2474b6553f4bbf396f824021a4052fb6672742..9b6a7ad3bb1bf6eab69e153d6f88355f403a2bcd#r68489976 --- lib/daemon.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/daemon.js b/lib/daemon.js index 45b52989..4c04d0eb 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -10,6 +10,7 @@ import Configuration from './Configuration'; import Importer from './Importer'; import WatchmanFileCache from './WatchmanFileCache'; import rerouteConsoleLog from './rerouteConsoleLog'; +import version from './version'; const pathToLogFile = path.join(os.tmpdir(), 'importjs.log'); const originalConsoleLog = rerouteConsoleLog(pathToLogFile); @@ -34,7 +35,8 @@ export default function daemon() { prefixes: ['timestamp', 'level'], staticPrefixes: [`PID:${commander.parentPid}`], }); - originalConsoleLog(`DAEMON active. Logs will go to: ${pathToLogFile}`); + originalConsoleLog( + `ImportJS (v${version()}) DAEMON active. Logs will go to: ${pathToLogFile}`); WatchmanFileCache.initialize().then(() => { loglevel.info('WATCHMAN file cache is enabled');