Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: eslint --init installs wrong dependencies of popular styles (fixes #7338) #8713

Merged
merged 3 commits into from Jun 16, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 26 additions & 22 deletions lib/config/config-initializer.js
Expand Up @@ -62,42 +62,46 @@ function writeFile(config, format) {
* @returns {void}
*/
function installModules(config) {
let modules = [];
const modules = {};

// Create a list of modules which should be installed based on config
if (config.plugins) {
modules = modules.concat(config.plugins.map(name => `eslint-plugin-${name}`));
for (const plugin of config.plugins) {
modules[`eslint-plugin-${plugin}`] = "latest";
}
}
if (config.extends && config.extends.indexOf("eslint:") === -1) {
modules.push(`eslint-config-${config.extends}`);
const moduleName = `eslint-config-${config.extends}`;

log.info(`Checking peerDependencies of ${moduleName}`);
modules[moduleName] = "latest";
Object.assign(
modules,
npmUtil.fetchPeerDependencies(`${moduleName}@latest`)
);
}

// Determine which modules are already installed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pre-existing comment seems incorrect

if (modules.length === 0) {
if (Object.keys(modules).length === 0) {
return;
}

// Add eslint to list in case user does not have it installed locally
modules.unshift("eslint");
modules.eslint = modules.eslint || "latest";

const installStatus = npmUtil.checkDevDeps(modules);
const installStatus = npmUtil.checkDevDeps(["eslint"]);

// Install packages which aren't already installed
const modulesToInstall = Object.keys(installStatus).filter(module => {
const notInstalled = installStatus[module] === false;
if (installStatus.eslint === false) {
log.info("Local ESLint installation not found.");
config.installedESLint = true;
}

if (module === "eslint" && notInstalled) {
log.info("Local ESLint installation not found.");
config.installedESLint = true;
}
// Install packages
const modulesToInstall = Object.keys(modules).map(name => `${name}@${modules[name]}`);

return notInstalled;
});
log.info(`Installing ${modulesToInstall.join(", ")}`);

if (modulesToInstall.length > 0) {
log.info(`Installing ${modulesToInstall.join(", ")}`);
npmUtil.installSyncSaveDev(modulesToInstall);
}
npmUtil.installSyncSaveDev(modulesToInstall);
}

/**
Expand Down Expand Up @@ -265,9 +269,9 @@ function processAnswers(answers) {
function getConfigForStyleGuide(guide) {
const guides = {
google: { extends: "google" },
airbnb: { extends: "airbnb", plugins: ["react", "jsx-a11y", "import"] },
"airbnb-base": { extends: "airbnb-base", plugins: ["import"] },
standard: { extends: "standard", plugins: ["standard", "promise"] }
airbnb: { extends: "airbnb" },
"airbnb-base": { extends: "airbnb-base" },
standard: { extends: "standard" }
};

if (!guides[guide]) {
Expand Down
15 changes: 15 additions & 0 deletions lib/util/npm-util.js
Expand Up @@ -56,6 +56,20 @@ function installSyncSaveDev(packages) {
childProcess.execSync(`npm i --save-dev ${packages}`, { stdio: "inherit", encoding: "utf8" });
}

/**
* Fetch `peerDependencies` of the given package by `npm show` command.
* @param {string} packageName The package name to fetch peerDependencies.
* @returns {string[]} Gotten peerDependencies.
*/
function fetchPeerDependencies(packageName) {
const fetchedText = childProcess.execSync(
`npm show --json ${packageName} peerDependencies`,
{ encoding: "utf8" }
).trim();

return JSON.parse(fetchedText || "{}");
}

/**
* Check whether node modules are include in a project's package.json.
*
Expand Down Expand Up @@ -140,6 +154,7 @@ function checkPackageJson(startDir) {

module.exports = {
installSyncSaveDev,
fetchPeerDependencies,
checkDeps,
checkDevDeps,
checkPackageJson
Expand Down
38 changes: 33 additions & 5 deletions tests/lib/config/config-initializer.js
Expand Up @@ -32,6 +32,7 @@ describe("configInitializer", () => {
let fixtureDir,
npmCheckStub,
npmInstallStub,
npmFetchPeerDependenciesStub,
init;

const log = {
Expand Down Expand Up @@ -75,6 +76,14 @@ describe("configInitializer", () => {
status[pkg] = false;
return status;
}, {}));
npmFetchPeerDependenciesStub = sinon
.stub(npmUtil, "fetchPeerDependencies")
.returns({
eslint: "^3.19.0",
"eslint-plugin-jsx-a11y": "^5.0.1",
"eslint-plugin-import": "^2.2.0",
"eslint-plugin-react": "^7.0.1"
});
init = proxyquire("../../../lib/config/config-initializer", requireStubs);
});

Expand All @@ -83,6 +92,7 @@ describe("configInitializer", () => {
log.error.reset();
npmInstallStub.restore();
npmCheckStub.restore();
npmFetchPeerDependenciesStub.restore();
});

after(() => {
Expand Down Expand Up @@ -185,19 +195,19 @@ describe("configInitializer", () => {
it("should support the airbnb style guide", () => {
const config = init.getConfigForStyleGuide("airbnb");

assert.deepEqual(config, { extends: "airbnb", installedESLint: true, plugins: ["react", "jsx-a11y", "import"] });
assert.deepEqual(config, { extends: "airbnb", installedESLint: true });
});

it("should support the airbnb base style guide", () => {
const config = init.getConfigForStyleGuide("airbnb-base");

assert.deepEqual(config, { extends: "airbnb-base", installedESLint: true, plugins: ["import"] });
assert.deepEqual(config, { extends: "airbnb-base", installedESLint: true });
});

it("should support the standard style guide", () => {
const config = init.getConfigForStyleGuide("standard");

assert.deepEqual(config, { extends: "standard", installedESLint: true, plugins: ["standard", "promise"] });
assert.deepEqual(config, { extends: "standard", installedESLint: true });
});

it("should throw when encountering an unsupported style guide", () => {
Expand All @@ -209,13 +219,31 @@ describe("configInitializer", () => {
it("should install required sharable config", () => {
init.getConfigForStyleGuide("google");
assert(npmInstallStub.calledOnce);
assert.deepEqual(npmInstallStub.firstCall.args[0][1], "eslint-config-google");
assert(npmInstallStub.firstCall.args[0].some(name => name.startsWith("eslint-config-google@")));
});

it("should install ESLint if not installed locally", () => {
init.getConfigForStyleGuide("google");
assert(npmInstallStub.calledOnce);
assert.deepEqual(npmInstallStub.firstCall.args[0][0], "eslint");
assert(npmInstallStub.firstCall.args[0].some(name => name.startsWith("eslint@")));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be a good idea to assert that the peerDependencies are also installed.

});

it("should install peerDependencies of the sharable config", () => {
init.getConfigForStyleGuide("airbnb");

assert(npmFetchPeerDependenciesStub.calledOnce);
assert(npmFetchPeerDependenciesStub.firstCall.args[0] === "eslint-config-airbnb@latest");
assert(npmInstallStub.calledOnce);
assert.deepEqual(
npmInstallStub.firstCall.args[0],
[
"eslint-config-airbnb@latest",
"eslint@^3.19.0",
"eslint-plugin-jsx-a11y@^5.0.1",
"eslint-plugin-import@^2.2.0",
"eslint-plugin-react@^7.0.1"
]
);
});
});

Expand Down
11 changes: 11 additions & 0 deletions tests/lib/util/npm-util.js
Expand Up @@ -187,4 +187,15 @@ describe("npmUtil", () => {
stub.restore();
});
});

describe("fetchPeerDependencies()", () => {
it("should execute 'npm show --json <packageName> peerDependencies' command", () => {
const stub = sandbox.stub(childProcess, "execSync").returns("");

npmUtil.fetchPeerDependencies("desired-package");
assert(stub.calledOnce);
assert.equal(stub.firstCall.args[0], "npm show --json desired-package peerDependencies");
stub.restore();
});
});
});