From 8ca1a74aa6a05866b7de3b29713f3521bc5313d9 Mon Sep 17 00:00:00 2001 From: Steven Hargrove Date: Wed, 7 Feb 2018 19:38:04 -0500 Subject: [PATCH 1/5] replaced read-pkg-up package locator with caching behavior --- package.json | 3 +- src/core/createPackageLocator.js | 69 +++++++++++++++++++++++++ src/rules/no-extraneous-dependencies.js | 14 +++-- 3 files changed, 79 insertions(+), 7 deletions(-) create mode 100644 src/core/createPackageLocator.js diff --git a/package.json b/package.json index 6282f6c2be..dfb16a697f 100644 --- a/package.json +++ b/package.json @@ -87,8 +87,7 @@ "eslint-module-utils": "^2.1.1", "has": "^1.0.1", "lodash": "^4.17.4", - "minimatch": "^3.0.3", - "read-pkg-up": "^2.0.0" + "minimatch": "^3.0.3" }, "nyc": { "require": [ diff --git a/src/core/createPackageLocator.js b/src/core/createPackageLocator.js new file mode 100644 index 0000000000..7e52889f33 --- /dev/null +++ b/src/core/createPackageLocator.js @@ -0,0 +1,69 @@ +import path from 'path' +import fs from 'fs' +import { EOL } from 'os' + +class PackageLocator { + readPackageSync(location) { + return { + result: JSON.parse(fs.readFileSync(location, 'utf8')), + location, + } + } + + readPackageUpSync(dirname) { + const locations = [] + + do { + try { + const location = path.join(dirname, 'package.json') + locations.push(location) + + return this.readPackageSync(location) + } catch (err) { + // if not found, allopw search to continue + if (err.code !== 'ENOENT') throw err + } + } while (dirname !== (dirname = path.dirname(dirname))) + + const notFoundError = new Error(`No such file or directory: ${EOL}${locations.join(EOL)}`) + notFoundError.code = 'ENOENT' + + throw notFoundError + } +} + +class CachedPackageLocator extends PackageLocator { + constructor() { + super() + + this.store = {} + this.requests = new Map() + } + + readPackageSync(location) { + const cached = this.store[location] + if (cached) return cached + + const response = super.readPackageSync(location) + this.store[location] = response.result + return response + } + + readPackageUpSync(dirname) { + const cached = this.requests.get(dirname) + if (cached) return cached + + const response = super.readPackageUpSync(dirname) + this.requests.set(dirname, this.store[response.location.toLowerCase]) + return response + } + + clear() { + this.requests.clear() + this.store = {} + } +} + +export default function createPackageLocator(cache) { + return cache ? new CachedPackageLocator() : new PackageLocator() +} diff --git a/src/rules/no-extraneous-dependencies.js b/src/rules/no-extraneous-dependencies.js index bb684e4480..57eb28cc37 100644 --- a/src/rules/no-extraneous-dependencies.js +++ b/src/rules/no-extraneous-dependencies.js @@ -1,17 +1,21 @@ import path from 'path' -import fs from 'fs' -import readPkgUp from 'read-pkg-up' import minimatch from 'minimatch' import resolve from 'eslint-module-utils/resolve' import importType from '../core/importType' import isStaticRequire from '../core/staticRequire' +import createPackageLocator from '../core/createPackageLocator' import docsUrl from '../docsUrl' +const cachedPackageLocator = createPackageLocator(true) + function getDependencies(context, packageDir) { try { - const packageContent = packageDir - ? JSON.parse(fs.readFileSync(path.join(packageDir, 'package.json'), 'utf8')) - : readPkgUp.sync({cwd: context.getFilename(), normalize: false}).pkg + cachedPackageLocator.clear() + const packageContent = ( + packageDir + ? cachedPackageLocator.readPackageSync(path.join(packageDir, 'package.json')) + : cachedPackageLocator.readPackageUpSync(path.dirname(context.getFilename())) + ).result if (!packageContent) { return null From 33d0c9f8b098a802320a28563511708d9880e1ef Mon Sep 17 00:00:00 2001 From: Steven Hargrove Date: Thu, 8 Feb 2018 06:49:38 -0500 Subject: [PATCH 2/5] cleanup caching solution, adjusted cooresponding tests to address caused by slight change for error reporting --- src/core/CachedPackageLocator.js | 52 ++++++++++++++ src/core/createPackageLocator.js | 69 ------------------- src/rules/no-extraneous-dependencies.js | 61 +++++++--------- tests/src/rules/no-extraneous-dependencies.js | 43 +++++++++--- 4 files changed, 110 insertions(+), 115 deletions(-) create mode 100644 src/core/CachedPackageLocator.js delete mode 100644 src/core/createPackageLocator.js diff --git a/src/core/CachedPackageLocator.js b/src/core/CachedPackageLocator.js new file mode 100644 index 0000000000..e5e55adad7 --- /dev/null +++ b/src/core/CachedPackageLocator.js @@ -0,0 +1,52 @@ +import path from 'path' +import fs from 'fs' +import os from 'os' + +export default class CachedPackageLocator { + constructor() { + this.store = {} + } + + readUpSync(context, dirname, immediate) { + const locations = [] + + do { + const location = path.join(dirname, 'package.json') + + try { + locations.push(location) + if (this.store[location]) { + return this.store[location] + } + if (this.store[location] === null) { + continue + } + + return this.store[location] = JSON.parse(fs.readFileSync(location, 'utf8')) + } catch (err) { + if (err.code === 'ENOENT') { + this.store[location] = null + + if (immediate) { + context.report({ + message: 'Could not find package.json file: ' + location, + loc: { line: 0, column: 0 }, + }) + return + } + } else if (err instanceof SyntaxError) { + context.report({ + message: 'Could not parse package.json file: ' + err.message + ': ' + location, + loc: { line: 0, column: 0 }, + }) + return + } + } + } while (dirname !== (dirname = path.dirname(dirname))) + + context.report({ + message: `Could not find package.json files: ${os.EOL}${locations.join(os.EOL)}`, + loc: { line: 0, column: 0 }, + }) + } +} diff --git a/src/core/createPackageLocator.js b/src/core/createPackageLocator.js deleted file mode 100644 index 7e52889f33..0000000000 --- a/src/core/createPackageLocator.js +++ /dev/null @@ -1,69 +0,0 @@ -import path from 'path' -import fs from 'fs' -import { EOL } from 'os' - -class PackageLocator { - readPackageSync(location) { - return { - result: JSON.parse(fs.readFileSync(location, 'utf8')), - location, - } - } - - readPackageUpSync(dirname) { - const locations = [] - - do { - try { - const location = path.join(dirname, 'package.json') - locations.push(location) - - return this.readPackageSync(location) - } catch (err) { - // if not found, allopw search to continue - if (err.code !== 'ENOENT') throw err - } - } while (dirname !== (dirname = path.dirname(dirname))) - - const notFoundError = new Error(`No such file or directory: ${EOL}${locations.join(EOL)}`) - notFoundError.code = 'ENOENT' - - throw notFoundError - } -} - -class CachedPackageLocator extends PackageLocator { - constructor() { - super() - - this.store = {} - this.requests = new Map() - } - - readPackageSync(location) { - const cached = this.store[location] - if (cached) return cached - - const response = super.readPackageSync(location) - this.store[location] = response.result - return response - } - - readPackageUpSync(dirname) { - const cached = this.requests.get(dirname) - if (cached) return cached - - const response = super.readPackageUpSync(dirname) - this.requests.set(dirname, this.store[response.location.toLowerCase]) - return response - } - - clear() { - this.requests.clear() - this.store = {} - } -} - -export default function createPackageLocator(cache) { - return cache ? new CachedPackageLocator() : new PackageLocator() -} diff --git a/src/rules/no-extraneous-dependencies.js b/src/rules/no-extraneous-dependencies.js index 57eb28cc37..eb45f80fce 100644 --- a/src/rules/no-extraneous-dependencies.js +++ b/src/rules/no-extraneous-dependencies.js @@ -3,46 +3,28 @@ import minimatch from 'minimatch' import resolve from 'eslint-module-utils/resolve' import importType from '../core/importType' import isStaticRequire from '../core/staticRequire' -import createPackageLocator from '../core/createPackageLocator' +import CachedPackageLocator from '../core/CachedPackageLocator' import docsUrl from '../docsUrl' -const cachedPackageLocator = createPackageLocator(true) +const packageLocator = new CachedPackageLocator() -function getDependencies(context, packageDir) { - try { - cachedPackageLocator.clear() - const packageContent = ( - packageDir - ? cachedPackageLocator.readPackageSync(path.join(packageDir, 'package.json')) - : cachedPackageLocator.readPackageUpSync(path.dirname(context.getFilename())) - ).result - - if (!packageContent) { - return null - } - - return { - dependencies: packageContent.dependencies || {}, - devDependencies: packageContent.devDependencies || {}, - optionalDependencies: packageContent.optionalDependencies || {}, - peerDependencies: packageContent.peerDependencies || {}, - } - } catch (e) { - if (packageDir && e.code === 'ENOENT') { - context.report({ - message: 'The package.json file could not be found.', - loc: { line: 0, column: 0 }, - }) - } - if (e.name === 'JSONError' || e instanceof SyntaxError) { - context.report({ - message: 'The package.json file could not be parsed: ' + e.message, - loc: { line: 0, column: 0 }, - }) - } +function hasKeys(obj = {}) { + return Object.keys(obj).length +} - return null - } +function getDependencies(context, packageDir) { + const { + dependencies = {}, + devDependencies = {}, + peerDependencies = {}, + optionalDependencies = {}, + } = packageLocator.readUpSync( + context, + packageDir || path.dirname(context.getFilename()), + packageDir + ) || {} + + return { dependencies, devDependencies, optionalDependencies, peerDependencies } } function missingErrorMessage(packageName) { @@ -140,7 +122,12 @@ module.exports = { const filename = context.getFilename() const deps = getDependencies(context, options.packageDir) - if (!deps) { + if (![ + deps.dependencies, + deps.devDependencies, + deps.peerDependencies, + deps.optionalDependencies, + ].some(hasKeys)) { return {} } diff --git a/tests/src/rules/no-extraneous-dependencies.js b/tests/src/rules/no-extraneous-dependencies.js index a8817931b1..7fb58222e4 100644 --- a/tests/src/rules/no-extraneous-dependencies.js +++ b/tests/src/rules/no-extraneous-dependencies.js @@ -1,17 +1,21 @@ import { test } from '../utils' -import * as path from 'path' -import * as fs from 'fs' +import path from 'path' +import fs from 'fs' +import os from 'os' import { RuleTester } from 'eslint' + const ruleTester = new RuleTester() , rule = require('rules/no-extraneous-dependencies') const packageDirWithSyntaxError = path.join(__dirname, '../../files/with-syntax-error') const packageFileWithSyntaxErrorMessage = (() => { + const location = path.join(packageDirWithSyntaxError, 'package.json') + try { - JSON.parse(fs.readFileSync(path.join(packageDirWithSyntaxError, 'package.json'))) + JSON.parse(fs.readFileSync(location)) } catch (error) { - return error.message + return error.message + ': ' + location } })() const packageDirWithFlowTyped = path.join(__dirname, '../../files/with-flow-typed') @@ -183,18 +187,39 @@ ruleTester.run('no-extraneous-dependencies', rule, { }), test({ code: 'import "bar"', - options: [{packageDir: path.join(__dirname, './doesn-exist/')}], + options: [{packageDir: path.join(__dirname, 'doesn-exist')}], errors: [{ ruleId: 'no-extraneous-dependencies', - message: 'The package.json file could not be found.', - }], + message: 'Could not find package.json file: ' + path.join(path.join(__dirname, 'doesn-exist', 'package.json')), + }] + }), + test({ + code: 'import "bar"', + options: [{packageDir: '/does/not/exist'}], + errors: [{ + ruleId: 'no-extraneous-dependencies', + message: 'Could not find package.json file: /does/not/exist/package.json', + }] + }), + test({ + code: 'import "bar"', + filename: path.join('/does/not/exist', 'file.js'), + errors: [{ + ruleId: 'no-extraneous-dependencies', + message: 'Could not find package.json files: ' + os.EOL + [ + '/does/not/exist/package.json', + '/does/not/package.json', + '/does/package.json', + '/package.json', + ].join(os.EOL), + }] }), test({ - code: 'import foo from "foo"', + code: 'import "foo"', options: [{packageDir: packageDirWithSyntaxError}], errors: [{ ruleId: 'no-extraneous-dependencies', - message: 'The package.json file could not be parsed: ' + packageFileWithSyntaxErrorMessage, + message: 'Could not parse package.json file: ' + packageFileWithSyntaxErrorMessage, }], }), ], From 35be90388ab83357c4c88b30678484c2d1d25bf1 Mon Sep 17 00:00:00 2001 From: Steven Hargrove Date: Thu, 8 Feb 2018 18:19:40 -0500 Subject: [PATCH 3/5] only cache/return what's needed, removed redundant conditionals --- src/core/CachedPackageLocator.js | 9 ++++--- src/rules/no-extraneous-dependencies.js | 34 ++++++++++++------------- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/src/core/CachedPackageLocator.js b/src/core/CachedPackageLocator.js index e5e55adad7..1d44147b72 100644 --- a/src/core/CachedPackageLocator.js +++ b/src/core/CachedPackageLocator.js @@ -7,22 +7,25 @@ export default class CachedPackageLocator { this.store = {} } - readUpSync(context, dirname, immediate) { + readUpSync(context, dirname, immediate, reduce) { const locations = [] do { const location = path.join(dirname, 'package.json') try { - locations.push(location) if (this.store[location]) { return this.store[location] } + + locations.push(location) if (this.store[location] === null) { continue } - return this.store[location] = JSON.parse(fs.readFileSync(location, 'utf8')) + return this.store[location] = reduce( + JSON.parse(fs.readFileSync(location, 'utf8')) + ) } catch (err) { if (err.code === 'ENOENT') { this.store[location] = null diff --git a/src/rules/no-extraneous-dependencies.js b/src/rules/no-extraneous-dependencies.js index eb45f80fce..2f303e6ed6 100644 --- a/src/rules/no-extraneous-dependencies.js +++ b/src/rules/no-extraneous-dependencies.js @@ -8,23 +8,28 @@ import docsUrl from '../docsUrl' const packageLocator = new CachedPackageLocator() -function hasKeys(obj = {}) { +function keyLength(obj) { return Object.keys(obj).length } +function reducePackage({ + dependencies = {}, + devDependencies = {}, + peerDependencies = {}, + optionalDependencies = {}, +}) => { + if ([dependencies, devDependencies, peerDependencies, optionalDependencies].some(keyLength)) { + return { dependencies, devDependencies, peerDependencies, optionalDependencies } + } +} + function getDependencies(context, packageDir) { - const { - dependencies = {}, - devDependencies = {}, - peerDependencies = {}, - optionalDependencies = {}, - } = packageLocator.readUpSync( + return packageLocator.readUpSync( context, packageDir || path.dirname(context.getFilename()), - packageDir - ) || {} - - return { dependencies, devDependencies, optionalDependencies, peerDependencies } + packageDir, + reducePackage, + ) } function missingErrorMessage(packageName) { @@ -122,12 +127,7 @@ module.exports = { const filename = context.getFilename() const deps = getDependencies(context, options.packageDir) - if (![ - deps.dependencies, - deps.devDependencies, - deps.peerDependencies, - deps.optionalDependencies, - ].some(hasKeys)) { + if (!deps) { return {} } From c40029f7155be703e454538de8247178a8ecffc0 Mon Sep 17 00:00:00 2001 From: Steven Hargrove Date: Fri, 9 Feb 2018 21:35:33 -0500 Subject: [PATCH 4/5] more cleanup, added tests to check caching behavior for CachedPackageLocator --- src/core/CachedPackageLocator.js | 46 ++++-- src/rules/no-extraneous-dependencies.js | 36 +---- tests/src/core/CachedPackageLocator.js | 185 ++++++++++++++++++++++++ 3 files changed, 227 insertions(+), 40 deletions(-) create mode 100644 tests/src/core/CachedPackageLocator.js diff --git a/src/core/CachedPackageLocator.js b/src/core/CachedPackageLocator.js index 1d44147b72..420463e7cf 100644 --- a/src/core/CachedPackageLocator.js +++ b/src/core/CachedPackageLocator.js @@ -1,31 +1,50 @@ import path from 'path' -import fs from 'fs' import os from 'os' +import fs from 'fs' + +function notEmpty(obj) { + return Object.keys(obj).length +} + +function reducePackage({ + dependencies = {}, + devDependencies = {}, + peerDependencies = {}, + optionalDependencies = {}, +} = {}) { + if ([dependencies, devDependencies, peerDependencies, optionalDependencies].some(notEmpty)) { + return { dependencies, devDependencies, peerDependencies, optionalDependencies } + } + return null +} export default class CachedPackageLocator { constructor() { this.store = {} } - readUpSync(context, dirname, immediate, reduce) { + readUpSync(context, dirname, immediate) { const locations = [] - do { const location = path.join(dirname, 'package.json') - try { - if (this.store[location]) { - return this.store[location] - } + if (this.store[location]) { + return this.store[location] + } - locations.push(location) - if (this.store[location] === null) { - continue - } + locations.push(location) + if (this.store[location] === null) { + continue + } - return this.store[location] = reduce( + try { + this.store[location] = reducePackage( JSON.parse(fs.readFileSync(location, 'utf8')) ) + + if (this.store[location]) { + return this.store[location] + } } catch (err) { if (err.code === 'ENOENT') { this.store[location] = null @@ -43,6 +62,9 @@ export default class CachedPackageLocator { loc: { line: 0, column: 0 }, }) return + } else { + // dont swallow unknown error + throw err } } } while (dirname !== (dirname = path.dirname(dirname))) diff --git a/src/rules/no-extraneous-dependencies.js b/src/rules/no-extraneous-dependencies.js index 2f303e6ed6..e37601f4d8 100644 --- a/src/rules/no-extraneous-dependencies.js +++ b/src/rules/no-extraneous-dependencies.js @@ -8,30 +8,6 @@ import docsUrl from '../docsUrl' const packageLocator = new CachedPackageLocator() -function keyLength(obj) { - return Object.keys(obj).length -} - -function reducePackage({ - dependencies = {}, - devDependencies = {}, - peerDependencies = {}, - optionalDependencies = {}, -}) => { - if ([dependencies, devDependencies, peerDependencies, optionalDependencies].some(keyLength)) { - return { dependencies, devDependencies, peerDependencies, optionalDependencies } - } -} - -function getDependencies(context, packageDir) { - return packageLocator.readUpSync( - context, - packageDir || path.dirname(context.getFilename()), - packageDir, - reducePackage, - ) -} - function missingErrorMessage(packageName) { return `'${packageName}' should be listed in the project's dependencies. ` + `Run 'npm i -S ${packageName}' to add it` @@ -122,10 +98,14 @@ module.exports = { ], }, - create: function (context) { + create(context) { const options = context.options[0] || {} const filename = context.getFilename() - const deps = getDependencies(context, options.packageDir) + const deps = packageLocator.readUpSync( + context, + options.packageDir || path.dirname(context.getFilename()), + typeof options.packageDir !== 'undefined' + ) if (!deps) { return {} @@ -139,10 +119,10 @@ module.exports = { // todo: use module visitor from module-utils core return { - ImportDeclaration: function (node) { + ImportDeclaration(node) { reportIfMissing(context, deps, depsOptions, node, node.source.value) }, - CallExpression: function handleRequires(node) { + CallExpression(node) { if (isStaticRequire(node)) { reportIfMissing(context, deps, depsOptions, node, node.arguments[0].value) } diff --git a/tests/src/core/CachedPackageLocator.js b/tests/src/core/CachedPackageLocator.js new file mode 100644 index 0000000000..3bfef5164b --- /dev/null +++ b/tests/src/core/CachedPackageLocator.js @@ -0,0 +1,185 @@ +import fs from 'fs' +import sinon from 'sinon' +import { expect } from 'chai' + +import CachedPackageLocator from '../../../src/core/CachedPackageLocator' + +describe('CachedPackageLocator.readUpSync()', function () { + let sandbox + let packageLocator + + const withUnexpectedTokenStr = '{{"name":"foo"}' + const withUnexpectedEndStr = '{"name":"foo"' + const withDeps = { + dependencies: { 'prop-types': '~15.0.0' }, + devDependencies: { 'webpack': '^2.0.0' }, + peerDependencies: { 'react': '>=15.0.0' }, + optionalDependencies: { 'fs-events': '*' }, + } + const withDepsStr = JSON.stringify(withDeps) + const withDepsExtraFieldsStr = JSON.stringify( + Object.assign({}, withDeps, { + name: 'not-needed', + description: 'foo bar', + }) + ) + const context = { + report: sinon.spy(), + } + + before(function () { + sandbox = sinon.sandbox.create() + sandbox.stub(fs, 'readFileSync').reset() + }) + + after(function () { + sandbox.restore() + }) + + beforeEach(function () { + fs.readFileSync.throws({ code: 'ENOENT' }) + packageLocator = new CachedPackageLocator() + }) + + afterEach(function () { + context.report.reset() + sandbox.reset() + }) + + it('should not repeat fs.readFileSync on stored locations', function () { + fs.readFileSync.withArgs('/a/package.json').returns(withDepsStr) + + expect(packageLocator.readUpSync(context, '/a/b')).to.deep.equal(withDeps) + sinon.assert.callCount(fs.readFileSync, 2) + expect(packageLocator.readUpSync(context, '/a')).to.deep.equal(withDeps) + sinon.assert.callCount(fs.readFileSync, 2) + expect(packageLocator.store).to.deep.equal({ + '/a/b/package.json': null, + '/a/package.json': withDeps, + }) + + expect(packageLocator.readUpSync(context, '/x')).to.be.undefined + sinon.assert.callCount(fs.readFileSync, 4) + expect(packageLocator.readUpSync(context, '/x')).to.be.undefined + sinon.assert.callCount(fs.readFileSync, 4) + expect(packageLocator.store).to.deep.equal({ + '/x/package.json': null, + '/a/b/package.json': null, + '/a/package.json': withDeps, + '/package.json': null, + }) + + expect(packageLocator.readUpSync(context, '/x/y/z')).to.be.undefined + sinon.assert.callCount(fs.readFileSync, 6) + expect(packageLocator.readUpSync(context, '/x/y/z')).to.be.undefined + sinon.assert.callCount(fs.readFileSync, 6) + expect(packageLocator.store).to.deep.equal({ + '/x/y/z/package.json': null, + '/x/y/package.json': null, + '/x/package.json': null, + '/a/b/package.json': null, + '/a/package.json': withDeps, + '/package.json': null, + }) + + expect(packageLocator.readUpSync(context, '/x/w')).to.be.undefined + sinon.assert.callCount(fs.readFileSync, 7) + expect(packageLocator.readUpSync(context, '/x/w')).to.be.undefined + sinon.assert.callCount(fs.readFileSync, 7) + + expect(packageLocator.store).to.deep.equal({ + '/x/y/z/package.json': null, + '/x/y/package.json': null, + '/x/w/package.json': null, + '/x/package.json': null, + '/a/b/package.json': null, + '/a/package.json': withDeps, + '/package.json': null, + }) + }) + + it('should only store and return dependency fields', function () { + fs.readFileSync.withArgs('/package.json').returns(withDepsExtraFieldsStr) + expect(packageLocator.readUpSync(context, '/')).to.deep.equal(withDeps) + expect(packageLocator.store).to.deep.equal({ + '/package.json': withDeps, + }) + sinon.assert.calledOnce(fs.readFileSync) + }) + + it('should locate first available', function () { + fs.readFileSync.withArgs('/a/b/package.json').returns(withDepsStr) + expect(packageLocator.readUpSync(context, '/a/b')).to.deep.equal(withDeps) + expect(packageLocator.store).to.deep.equal({ + '/a/b/package.json': withDeps, + }) + sinon.assert.notCalled(context.report) + }) + + it('should locate last available', function () { + fs.readFileSync.withArgs('/package.json').returns(withDepsStr) + expect(packageLocator.readUpSync(context, '/a/b/c/d/e/f')).to.deep.equal(withDeps) + expect(packageLocator.store).to.deep.equal({ + '/a/b/c/d/e/f/package.json': null, + '/a/b/c/d/e/package.json': null, + '/a/b/c/d/package.json': null, + '/a/b/c/package.json': null, + '/a/b/package.json': null, + '/a/package.json': null, + '/package.json': withDeps, + }) + sinon.assert.notCalled(context.report) + }) + + it('should store package.json with empty deps as null', function () { + fs.readFileSync.withArgs('/package.json').returns('{}') + expect(packageLocator.readUpSync(context, '/')).to.be.undefined + expect(packageLocator.store).to.deep.equal({ + '/package.json': null, + }) + sinon.assert.calledOnce(context.report) + }) + + it('should not store JSON.parse failures', function () { + fs.readFileSync + .withArgs('/package.json').returns(withDepsStr) + .withArgs('/a/package.json').returns(withUnexpectedTokenStr) + .withArgs('/a/b/package.json').returns(withUnexpectedEndStr) + expect(packageLocator.readUpSync(context, '/a')).to.be.undefined + expect(packageLocator.store).to.be.empty + expect(packageLocator.readUpSync(context, '/a/b/c/d')).to.be.undefined + expect(packageLocator.store).to.deep.equal({ + '/a/b/c/d/package.json': null, + '/a/b/c/package.json': null, + }) + sinon.assert.callCount(context.report, 2) + }) + + it('should store failed locations as null', function () { + expect(packageLocator.readUpSync(context, '/does/not/exist')).to.be.undefined + expect(packageLocator.store).to.deep.equal({ + '/does/not/exist/package.json': null, + '/does/not/package.json': null, + '/does/package.json': null, + '/package.json': null, + }) + sinon.assert.calledOnce(context.report) + }) + + it('immediate=true should halt on first failed location', function () { + expect(packageLocator.readUpSync(context, '/does/not/exist', true)).to.be.undefined + expect(packageLocator.store).to.deep.equal({ + '/does/not/exist/package.json': null + }) + sinon.assert.calledOnce(context.report) + }) + + it('should throw unknown errors', function () { + fs.readFileSync.throws(new Error('Some unknown error')) + expect(() => { + packageLocator.readUpSync(context, '/does/not/exist', true) + }).to.throw('Some unknown error') + expect(packageLocator.store).to.empty + sinon.assert.notCalled(context.report) + }) +}) From 14fdf06e2154d6a538897399730f7ccdebde077b Mon Sep 17 00:00:00 2001 From: Steven Hargrove Date: Sat, 10 Feb 2018 16:20:57 -0500 Subject: [PATCH 5/5] moved package reducer to rule, to allow pkg locator to be more generic --- src/core/CachedPackageLocator.js | 20 +-------- src/rules/no-extraneous-dependencies.js | 20 ++++++++- tests/src/core/CachedPackageLocator.js | 59 ++++++++++++++++++------- 3 files changed, 64 insertions(+), 35 deletions(-) diff --git a/src/core/CachedPackageLocator.js b/src/core/CachedPackageLocator.js index 420463e7cf..3284d40ca9 100644 --- a/src/core/CachedPackageLocator.js +++ b/src/core/CachedPackageLocator.js @@ -2,28 +2,12 @@ import path from 'path' import os from 'os' import fs from 'fs' -function notEmpty(obj) { - return Object.keys(obj).length -} - -function reducePackage({ - dependencies = {}, - devDependencies = {}, - peerDependencies = {}, - optionalDependencies = {}, -} = {}) { - if ([dependencies, devDependencies, peerDependencies, optionalDependencies].some(notEmpty)) { - return { dependencies, devDependencies, peerDependencies, optionalDependencies } - } - - return null -} export default class CachedPackageLocator { constructor() { this.store = {} } - readUpSync(context, dirname, immediate) { + readUpSync(context, dirname, immediate, reduce) { const locations = [] do { const location = path.join(dirname, 'package.json') @@ -38,7 +22,7 @@ export default class CachedPackageLocator { } try { - this.store[location] = reducePackage( + this.store[location] = reduce( JSON.parse(fs.readFileSync(location, 'utf8')) ) diff --git a/src/rules/no-extraneous-dependencies.js b/src/rules/no-extraneous-dependencies.js index e37601f4d8..93fc99ebbc 100644 --- a/src/rules/no-extraneous-dependencies.js +++ b/src/rules/no-extraneous-dependencies.js @@ -8,6 +8,23 @@ import docsUrl from '../docsUrl' const packageLocator = new CachedPackageLocator() +function notEmpty(obj) { + return Object.keys(obj).length +} + +function reducePackage({ + dependencies = {}, + devDependencies = {}, + peerDependencies = {}, + optionalDependencies = {}, +} = {}) { + if ([dependencies, devDependencies, peerDependencies, optionalDependencies].some(notEmpty)) { + return { dependencies, devDependencies, peerDependencies, optionalDependencies } + } + + return null +} + function missingErrorMessage(packageName) { return `'${packageName}' should be listed in the project's dependencies. ` + `Run 'npm i -S ${packageName}' to add it` @@ -104,7 +121,8 @@ module.exports = { const deps = packageLocator.readUpSync( context, options.packageDir || path.dirname(context.getFilename()), - typeof options.packageDir !== 'undefined' + typeof options.packageDir !== 'undefined', + reducePackage ) if (!deps) { diff --git a/tests/src/core/CachedPackageLocator.js b/tests/src/core/CachedPackageLocator.js index 3bfef5164b..cc496ab241 100644 --- a/tests/src/core/CachedPackageLocator.js +++ b/tests/src/core/CachedPackageLocator.js @@ -4,6 +4,23 @@ import { expect } from 'chai' import CachedPackageLocator from '../../../src/core/CachedPackageLocator' +function notEmpty(obj) { + return Object.keys(obj).length +} + +function reduce({ + dependencies = {}, + devDependencies = {}, + peerDependencies = {}, + optionalDependencies = {}, +} = {}) { + if ([dependencies, devDependencies, peerDependencies, optionalDependencies].some(notEmpty)) { + return { dependencies, devDependencies, peerDependencies, optionalDependencies } + } + + return null +} + describe('CachedPackageLocator.readUpSync()', function () { let sandbox let packageLocator @@ -49,18 +66,20 @@ describe('CachedPackageLocator.readUpSync()', function () { it('should not repeat fs.readFileSync on stored locations', function () { fs.readFileSync.withArgs('/a/package.json').returns(withDepsStr) - expect(packageLocator.readUpSync(context, '/a/b')).to.deep.equal(withDeps) + expect(packageLocator.readUpSync(context, '/a/b', false, reduce)) + .to.deep.equal(withDeps) sinon.assert.callCount(fs.readFileSync, 2) - expect(packageLocator.readUpSync(context, '/a')).to.deep.equal(withDeps) + expect(packageLocator.readUpSync(context, '/a', false, reduce)) + .to.deep.equal(withDeps) sinon.assert.callCount(fs.readFileSync, 2) expect(packageLocator.store).to.deep.equal({ '/a/b/package.json': null, '/a/package.json': withDeps, }) - expect(packageLocator.readUpSync(context, '/x')).to.be.undefined + expect(packageLocator.readUpSync(context, '/x', false, reduce)).to.be.undefined sinon.assert.callCount(fs.readFileSync, 4) - expect(packageLocator.readUpSync(context, '/x')).to.be.undefined + expect(packageLocator.readUpSync(context, '/x', false, reduce)).to.be.undefined sinon.assert.callCount(fs.readFileSync, 4) expect(packageLocator.store).to.deep.equal({ '/x/package.json': null, @@ -69,9 +88,9 @@ describe('CachedPackageLocator.readUpSync()', function () { '/package.json': null, }) - expect(packageLocator.readUpSync(context, '/x/y/z')).to.be.undefined + expect(packageLocator.readUpSync(context, '/x/y/z', false, reduce)).to.be.undefined sinon.assert.callCount(fs.readFileSync, 6) - expect(packageLocator.readUpSync(context, '/x/y/z')).to.be.undefined + expect(packageLocator.readUpSync(context, '/x/y/z', false, reduce)).to.be.undefined sinon.assert.callCount(fs.readFileSync, 6) expect(packageLocator.store).to.deep.equal({ '/x/y/z/package.json': null, @@ -82,9 +101,9 @@ describe('CachedPackageLocator.readUpSync()', function () { '/package.json': null, }) - expect(packageLocator.readUpSync(context, '/x/w')).to.be.undefined + expect(packageLocator.readUpSync(context, '/x/w', false, reduce)).to.be.undefined sinon.assert.callCount(fs.readFileSync, 7) - expect(packageLocator.readUpSync(context, '/x/w')).to.be.undefined + expect(packageLocator.readUpSync(context, '/x/w', false, reduce)).to.be.undefined sinon.assert.callCount(fs.readFileSync, 7) expect(packageLocator.store).to.deep.equal({ @@ -100,7 +119,8 @@ describe('CachedPackageLocator.readUpSync()', function () { it('should only store and return dependency fields', function () { fs.readFileSync.withArgs('/package.json').returns(withDepsExtraFieldsStr) - expect(packageLocator.readUpSync(context, '/')).to.deep.equal(withDeps) + expect(packageLocator.readUpSync(context, '/', false, reduce)) + .to.deep.equal(withDeps) expect(packageLocator.store).to.deep.equal({ '/package.json': withDeps, }) @@ -109,7 +129,8 @@ describe('CachedPackageLocator.readUpSync()', function () { it('should locate first available', function () { fs.readFileSync.withArgs('/a/b/package.json').returns(withDepsStr) - expect(packageLocator.readUpSync(context, '/a/b')).to.deep.equal(withDeps) + expect(packageLocator.readUpSync(context, '/a/b', false, reduce)) + .to.deep.equal(withDeps) expect(packageLocator.store).to.deep.equal({ '/a/b/package.json': withDeps, }) @@ -118,7 +139,8 @@ describe('CachedPackageLocator.readUpSync()', function () { it('should locate last available', function () { fs.readFileSync.withArgs('/package.json').returns(withDepsStr) - expect(packageLocator.readUpSync(context, '/a/b/c/d/e/f')).to.deep.equal(withDeps) + expect(packageLocator.readUpSync(context, '/a/b/c/d/e/f', false, reduce)) + .to.deep.equal(withDeps) expect(packageLocator.store).to.deep.equal({ '/a/b/c/d/e/f/package.json': null, '/a/b/c/d/e/package.json': null, @@ -133,7 +155,8 @@ describe('CachedPackageLocator.readUpSync()', function () { it('should store package.json with empty deps as null', function () { fs.readFileSync.withArgs('/package.json').returns('{}') - expect(packageLocator.readUpSync(context, '/')).to.be.undefined + expect(packageLocator.readUpSync(context, '/', false, reduce)) + .to.be.undefined expect(packageLocator.store).to.deep.equal({ '/package.json': null, }) @@ -145,9 +168,11 @@ describe('CachedPackageLocator.readUpSync()', function () { .withArgs('/package.json').returns(withDepsStr) .withArgs('/a/package.json').returns(withUnexpectedTokenStr) .withArgs('/a/b/package.json').returns(withUnexpectedEndStr) - expect(packageLocator.readUpSync(context, '/a')).to.be.undefined + expect(packageLocator.readUpSync(context, '/a', false)) + .to.be.undefined expect(packageLocator.store).to.be.empty - expect(packageLocator.readUpSync(context, '/a/b/c/d')).to.be.undefined + expect(packageLocator.readUpSync(context, '/a/b/c/d', false)) + .to.be.undefined expect(packageLocator.store).to.deep.equal({ '/a/b/c/d/package.json': null, '/a/b/c/package.json': null, @@ -156,7 +181,8 @@ describe('CachedPackageLocator.readUpSync()', function () { }) it('should store failed locations as null', function () { - expect(packageLocator.readUpSync(context, '/does/not/exist')).to.be.undefined + expect(packageLocator.readUpSync(context, '/does/not/exist', false)) + .to.be.undefined expect(packageLocator.store).to.deep.equal({ '/does/not/exist/package.json': null, '/does/not/package.json': null, @@ -167,7 +193,8 @@ describe('CachedPackageLocator.readUpSync()', function () { }) it('immediate=true should halt on first failed location', function () { - expect(packageLocator.readUpSync(context, '/does/not/exist', true)).to.be.undefined + expect(packageLocator.readUpSync(context, '/does/not/exist', true)) + .to.be.undefined expect(packageLocator.store).to.deep.equal({ '/does/not/exist/package.json': null })