From f8a6a4a3613121fd5d2af93db0910d4fd7828af9 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Thu, 26 May 2016 21:54:14 +0200 Subject: [PATCH 1/2] Add Node based file finder and use on Windows The default file finder (findMatchingFiles) executes a `find ... | egrep ...` command to find files matching a variable name. This doesn't work on Windows, so I'm exploring a native Node file finder as an alternative. I'm using glob [1], which is designed to work cross-platform. From what I can tell, this is the only big thing to take care of in order to make import-js work on Windows. I'm currently running tests on the sublime plugin to make things work cross platforms, and my initial tests are positive. [1]: https://github.com/isaacs/node-glob --- lib/__tests__/findMatchingFiles-test.js | 174 +++++++++++++----------- lib/findMatchingFiles.js | 51 ++++--- package.json | 1 + 3 files changed, 132 insertions(+), 94 deletions(-) diff --git a/lib/__tests__/findMatchingFiles-test.js b/lib/__tests__/findMatchingFiles-test.js index 4c2876bd..66e0c3ef 100644 --- a/lib/__tests__/findMatchingFiles-test.js +++ b/lib/__tests__/findMatchingFiles-test.js @@ -6,103 +6,121 @@ import rimraf from 'rimraf'; import findMatchingFiles from '../findMatchingFiles'; -describe('findMatchingFiles', () => { - const tmpDir = './tmp-findMatchingFiles'; - let subject; - let word; - let existingFiles; - let lookupPath; - - 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 +['node', 'find'].forEach((finder) => { + describe(`findMatchingFiles finder: ${finder}`, () => { + const tmpDir = './tmp-findMatchingFiles'; + let subject; + let word; + let existingFiles; + let lookupPath; + let originalPlatform; + + if (finder === 'node') { + beforeEach(() => { + originalPlatform = process.platform; + Object.defineProperty(process, 'platform', { + value: 'win32', + }); }); - return findMatchingFiles(lookupPath, word); - }; - }); - - afterEach((done) => { - rimraf(tmpDir, done); - }); + afterEach(() => { + Object.defineProperty(process, 'platform', { + value: originalPlatform, + }); + }); + } - describe('when there are no matching files', () => { beforeEach(() => { - existingFiles = [ - 'car/door.js', - ]; + 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); + }; }); - it('returns an empty array', () => { - expect(subject()).toEqual([]); + afterEach((done) => { + rimraf(tmpDir, done); }); - }); - describe('when the lookup path is empty', () => { - beforeEach(() => { - lookupPath = ''; - }); + describe('when there are no matching files', () => { + beforeEach(() => { + existingFiles = [ + 'car/door.js', + ]; + }); - it('throws an error', () => { - expect(subject).toThrowError(/empty/); + it('returns an empty array', () => { + expect(subject()).toEqual([]); + }); }); - }); - describe('when there are multiple files matching word', () => { - beforeEach(() => { - existingFiles = [ - 'bar/foo.js', - 'car/Foo.jsx', - 'tzar/foo/index.js', - 'car/door.js', - 'har-har/foo/package.json', - 'car/door/index.js', - 'react/foo/index.jsx', - ]; - }); + describe('when the lookup path is empty', () => { + beforeEach(() => { + lookupPath = ''; + }); - 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('throws an error', () => { + expect(subject).toThrowError(/empty/); + }); }); - }); - describe('when the word has camelCase', () => { - beforeEach(() => { - word = 'FooBar'; - }); + describe('when there are multiple files matching word', () => { + beforeEach(() => { + existingFiles = [ + 'bar/foo.js', + 'car/Foo.jsx', + 'tzar/foo/index.js', + 'car/door.js', + 'har-har/foo/package.json', + 'car/door/index.js', + 'react/foo/index.jsx', + ]; + }); - it('matches snake_case names', () => { - existingFiles = ['foo_bar.js']; - expect(subject()).toEqual([`${tmpDir}/foo_bar.js`]); + 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('matches camelCase names', () => { - existingFiles = ['fooBar.js']; - expect(subject()).toEqual([`${tmpDir}/fooBar.js`]); - }); + describe('when the word has camelCase', () => { + beforeEach(() => { + word = 'FooBar'; + }); - it('matches folder + file name', () => { - existingFiles = ['foo/Bar.js']; - expect(subject()).toEqual([`${tmpDir}/foo/Bar.js`]); - }); + it('matches snake_case names', () => { + existingFiles = ['foo_bar.js']; + expect(subject()).toEqual([`${tmpDir}/foo_bar.js`]); + }); + + it('matches camelCase names', () => { + existingFiles = ['fooBar.js']; + expect(subject()).toEqual([`${tmpDir}/fooBar.js`]); + }); - it('matches plural folder name + file name', () => { - existingFiles = ['foos/Bar.js']; - expect(subject()).toEqual([`${tmpDir}/foos/Bar.js`]); + it('matches folder + file name', () => { + existingFiles = ['foo/Bar.js']; + expect(subject()).toEqual([`${tmpDir}/foo/Bar.js`]); + }); + + it('matches plural folder name + file name', () => { + existingFiles = ['foos/Bar.js']; + expect(subject()).toEqual([`${tmpDir}/foos/Bar.js`]); + }); }); }); }); diff --git a/lib/findMatchingFiles.js b/lib/findMatchingFiles.js index 1840c2cf..a8d33acc 100644 --- a/lib/findMatchingFiles.js +++ b/lib/findMatchingFiles.js @@ -1,32 +1,20 @@ // @flow import childProcess from 'child_process'; +import glob from 'glob'; import formattedToRegex from './formattedToRegex'; -/** - * Finds files from the local file system matching the variable name. - */ -export default function findMatchingFiles( +function findMatchingFilesWithFind( lookupPath: string, - variableName: string + validFilesRegex: string ): Array { - if (lookupPath === '') { - // If lookupPath is an empty string, the `find` command will not work - // as desired so we bail early. - throw new Error(`lookup path cannot be empty (${lookupPath})`); - } - - const formattedVarName = formattedToRegex(variableName); - const egrepCommand = - `egrep -i \"(/|^)${formattedVarName}(/index)?(/package)?\.js.*\"`; - const findCommand = [ `find ${lookupPath}`, '-name "**.js*"', '-not -path "./node_modules/*"', ].join(' '); - const command = `${findCommand} | ${egrepCommand}`; + const command = `${findCommand} | egrep -i \"${validFilesRegex}\"`; // TODO switch to spawn so we can start processing the stream as it comes // in. @@ -45,3 +33,34 @@ export default function findMatchingFiles( // Filter out empty lines before returning return out.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)); +} + +/** + * Finds files from the local file system matching the variable name. + */ +export default function findMatchingFiles( + lookupPath: string, + variableName: string +): Array { + if (lookupPath === '') { + // If lookupPath is an empty string, the `find` command will not work + // as desired so we bail early. + throw new Error(`lookup path cannot be empty (${lookupPath})`); + } + + const formattedVarName = formattedToRegex(variableName); + const validFilesRegex = `(/|^)${formattedVarName}(/index)?(/package)?\\.js.*`; + + if (/^win/.test(process.platform)) { + return findMatchingFilesWithNode(lookupPath, validFilesRegex); + } + return findMatchingFilesWithFind(lookupPath, validFilesRegex); +} diff --git a/package.json b/package.json index 76100e95..648ed445 100644 --- a/package.json +++ b/package.json @@ -61,6 +61,7 @@ "StringScanner": "0.0.3", "commander": "^2.9.0", "eslint": "^2.8.0", + "glob": "^7.0.3", "lodash.escaperegexp": "^4.1.1", "lodash.flattendeep": "^4.1.0", "lodash.partition": "^4.2.0", From bc69bf830e231c8edaadf219ab176d0ea5c3ec2b Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Sat, 28 May 2016 22:26:54 +0200 Subject: [PATCH 2/2] Fix ignoring node_modules in findMatchingFilesWithNode The previous ignore pattern didn't work. Due to how we use a temp folder as the lookup path in our findMatchingFiles test, it's tricky to add a test for ignoring the node_modules folder. There's room for improvement: - we need a test - we should probably ignore nested node_modules folders as well If I have time, I'll come back to this soon to make it more robust. --- lib/findMatchingFiles.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/findMatchingFiles.js b/lib/findMatchingFiles.js index a8d33acc..267cf4f8 100644 --- a/lib/findMatchingFiles.js +++ b/lib/findMatchingFiles.js @@ -39,8 +39,9 @@ function findMatchingFilesWithNode( validFilesRegex: string ): Array { return glob.sync(`${lookupPath}/**/*.js*`, { - ignore: './node_modules/*', - }).filter((filePath: string): bool => new RegExp(validFilesRegex, 'i').test(filePath)); + ignore: './node_modules/**/*', + }).filter((filePath: string): bool => + new RegExp(validFilesRegex, 'i').test(filePath)); } /**