diff --git a/spec/rules-spec.js b/spec/rules-spec.js new file mode 100644 index 00000000..8808ad8a --- /dev/null +++ b/spec/rules-spec.js @@ -0,0 +1,83 @@ +'use babel' + +import Rules from '../src/rules' + +describe('The Rules class', () => { + describe('replaceRules', () => { + const ruleArray = [ + ['foo', { meta: { fixable: true } }], + ['bar', { meta: {} }] + ] + + it('adds new rules', () => { + const rules = new Rules() + expect(rules.getRules()).toEqual(new Map()) + rules.replaceRules(ruleArray) + expect(rules.getRules()).toEqual(new Map(ruleArray)) + }) + + it('removes old rules', () => { + const rules = new Rules() + rules.replaceRules(ruleArray) + expect(rules.getRules()).toEqual(new Map(ruleArray)) + rules.replaceRules([]) + expect(rules.getRules()).toEqual(new Map()) + }) + + it('updates the fixableRules list', () => { + const rules = new Rules() + expect(rules.getFixableRules()).toEqual([]) + rules.replaceRules(ruleArray) + expect(rules.getFixableRules()).toEqual(['foo']) + }) + + it('validates new rules', () => { + const rules = new Rules() + const testRuleReplace = (value, throws = true) => { + const test = () => { + rules.replaceRules(value) + } + if (throws) { + expect(test).toThrow() + } else { + expect(test).not.toThrow() + } + } + + // Invalid + testRuleReplace('foobar') + testRuleReplace({}) + testRuleReplace(null) + testRuleReplace(undefined) + testRuleReplace([[]]) + testRuleReplace([['foo']]) + testRuleReplace([['foo', 'bar']]) + testRuleReplace([['foo', []]]) + testRuleReplace([['foo', null]]) + testRuleReplace([['foo', undefined]]) + // Valid + testRuleReplace([], false) + testRuleReplace([['foo', {}]], false) + }) + }) + + describe('getRuleUrl', () => { + it('works with rules that define their own URL', () => { + const rules = new Rules() + rules.replaceRules([['foo', { meta: { docs: { url: 'bar' } } }]]) + expect(rules.getRuleUrl('foo')).toBe('bar') + }) + + it('works with rules defined in eslint-rule-documentation', () => { + const rules = new Rules() + const url = 'https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-duplicates.md' + expect(rules.getRuleUrl('import/no-duplicates')).toBe(url) + }) + + it('gives a fallback URL on how to add a rule URL', () => { + const rules = new Rules() + const url = 'https://github.com/jfmengels/eslint-rule-documentation/blob/master/contributing.md' + expect(rules.getRuleUrl('foo/bar')).toBe(url) + }) + }) +}) diff --git a/spec/worker-helpers-spec.js b/spec/worker-helpers-spec.js index 5c961928..fb40d493 100644 --- a/spec/worker-helpers-spec.js +++ b/spec/worker-helpers-spec.js @@ -211,4 +211,58 @@ describe('Worker Helpers', () => { rimraf.sync(tempDir) }) }) + + describe('getRules', () => { + it('works with the getRules function introduced in ESLint v4.15.0', () => { + const cliEngine = { + getRules: () => 'foo' + } + expect(Helpers.getRules(cliEngine)).toBe('foo') + }) + + it('works with the hidden linter in ESLint v4 before v4.15.0', () => { + const cliEngine = { + linter: { + getRules: () => 'foo' + } + } + expect(Helpers.getRules(cliEngine)).toBe('foo') + }) + + it('returns an empty Map for old ESLint versions', () => { + const cliEngine = {} + expect(Helpers.getRules(cliEngine)).toEqual(new Map()) + }) + }) + + describe('didRulesChange', () => { + const emptyRules = new Map() + const rules1 = new Map([['rule1', {}]]) + const rules2 = new Map([['rule1', {}], ['rule2', {}]]) + + it('returns false for empty Maps', () => { + const newRules = new Map() + expect(Helpers.didRulesChange(emptyRules, newRules)).toBe(false) + }) + + it('returns false when they are the same', () => { + expect(Helpers.didRulesChange(rules1, rules1)).toBe(false) + }) + + it('returns true when a new rule is added to an empty list', () => { + expect(Helpers.didRulesChange(emptyRules, rules1)).toBe(true) + }) + + it('returns true when the last rule is removed', () => { + expect(Helpers.didRulesChange(rules1, emptyRules)).toBe(true) + }) + + it('returns true when a new rule is added to an existing list', () => { + expect(Helpers.didRulesChange(rules1, rules2)).toBe(true) + }) + + it('returns true when a rule is removed from an existing list', () => { + expect(Helpers.didRulesChange(rules2, rules1)).toBe(true) + }) + }) }) diff --git a/src/helpers.js b/src/helpers.js index 91867b25..8cf91292 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -1,14 +1,13 @@ 'use babel' import { join } from 'path' -import ruleURI from 'eslint-rule-documentation' import { generateRange } from 'atom-linter' import cryptoRandomString from 'crypto-random-string' - // eslint-disable-next-line import/no-extraneous-dependencies, import/extensions import { Range } from 'atom' +import Rules from './rules' -const fixableRules = new Set() +export const rules = new Rules() /** * Start the worker process if it hasn't already been started @@ -72,10 +71,6 @@ export async function sendJob(worker, config) { }) } -export function getFixableRules() { - return Array.from(fixableRules.values()) -} - function validatePoint(textBuffer, line, col) { // Clip the given point to a valid one, and check if it equals the original if (!textBuffer.clipPosition([line, col]).isEqual([line, col])) { @@ -286,7 +281,7 @@ export async function processESLintMessages(messages, textEditor, showRule, work } if (ruleId) { - ret.url = ruleURI(ruleId).url + ret.url = rules.getRuleUrl(ruleId) } let range @@ -335,9 +330,8 @@ export async function processESLintMessages(messages, textEditor, showRule, work * @return {Promise} The messages transformed into Linter messages */ export async function processJobResponse(response, textEditor, showRule, worker) { - if (Object.prototype.hasOwnProperty.call(response, 'fixableRules')) { - fixableRules.clear() - response.fixableRules.forEach(rule => fixableRules.add(rule)) + if (Object.prototype.hasOwnProperty.call(response, 'updatedRules')) { + rules.replaceRules(response.updatedRules) } return processESLintMessages(response.messages, textEditor, showRule, worker) } diff --git a/src/main.js b/src/main.js index 2fc9f769..e6a4985f 100644 --- a/src/main.js +++ b/src/main.js @@ -274,7 +274,7 @@ module.exports = { } if (textEditor.isModified() && ignoreFixableRulesWhileTyping) { // Note that this list will only contain rules after the first lint job - rules = idsToIgnoredRules(helpers.getFixableRules()) + rules = idsToIgnoredRules(helpers.rules.getFixableRules()) } if (!this.worker) { diff --git a/src/rules.js b/src/rules.js new file mode 100644 index 00000000..ab685418 --- /dev/null +++ b/src/rules.js @@ -0,0 +1,88 @@ +import ruleURI from 'eslint-rule-documentation' + +// Private properties +const rules = Symbol('rules') +const validRulesArray = Symbol('validRulesArray') +const validateRulesArray = Symbol('validateRulesArray') + +/** + * Stores a list of rules from ESLint + */ +export default class Rules { + /** + * Instantiates a Rules object, optionally with an existing list of rules + * @param {Array} newRules Array of Arrays of the rule and properties + */ + constructor(newRules) { + if (Rules[validRulesArray](newRules)) { + this[rules] = new Map(newRules) + } else { + this[rules] = new Map() + } + } + + /** + * Process the updated rules into the local Map and call further update functions + * @param {Array} newRules Array of Arrays of the rule and properties + */ + replaceRules(newRules) { + Rules[validateRulesArray](newRules) + this[rules] = new Map(newRules) + } + + /** + * [getFixableRules description] + * @return {Array} The ruleIds of the currently known fixable rules + */ + getFixableRules() { + return Array.from(this[rules]).reduce((fixable, [rule, props]) => { + if (props.meta && props.meta.fixable) { + return [...fixable, rule] + } + return fixable + }, []) + } + + /** + * Get the URL of the documentation for a rule, either from the rule's own + * metadata, from eslint-rule-documentation's known rules, or the fallback URL + * on how to add it to eslint-rule-documentation. + * @param {String} ruleId The rule ID to get the documentation URL for + * @return {String} URL of the rule documentation + */ + getRuleUrl(ruleId) { + const props = this[rules].get(ruleId) + if (props && props.meta && props.meta.docs && props.meta.docs.url) { + // The rule has a documentation URL specified in its metadata + return props.meta.docs.url + } + + // The rule didn't specify a URL in its metadata, or was not currently known + // somehow. Attempt to determine a URL using eslint-rule-documentation. + return ruleURI(ruleId).url + } + + /** + * Return the known rules. + * @return {Map} The currently known rules + */ + getRules() { + return new Map(this[rules]) + } + + static [validRulesArray](rulesArray) { + if (!Array.isArray(rulesArray)) { + return false + } + return rulesArray.every(elem => + Array.isArray(elem) && elem.length === 2 && + typeof elem[0] === 'string' && typeof elem[1] === 'object' && + elem[1] !== null && !Array.isArray(elem[1])) + } + + static [validateRulesArray](rulesArray) { + if (!Rules[validRulesArray](rulesArray)) { + throw new Error('Given rules are not valid!') + } + } +} diff --git a/src/worker-helpers.js b/src/worker-helpers.js index 93ceb9fb..bdcfd47e 100644 --- a/src/worker-helpers.js +++ b/src/worker-helpers.js @@ -183,3 +183,41 @@ export function getCLIEngineOptions(type, config, rules, filePath, fileDir, give return cliEngineConfig } + +/** + * Gets the list of rules used for a lint job + * @param {Object} cliEngine The CLIEngine instance used for the lint job + * @return {Map} A Map of the rules used, rule names as keys, rule + * properties as the contents. + */ +export function getRules(cliEngine) { + // Pull the list of rules used directly from the CLIEngine + // Added in https://github.com/eslint/eslint/pull/9782 + if (Object.prototype.hasOwnProperty.call(cliEngine, 'getRules')) { + return cliEngine.getRules() + } + + // Attempt to use the internal (undocumented) `linter` instance attached to + // the CLIEngine to get the loaded rules (including plugin rules). + // Added in ESLint v4 + if (Object.prototype.hasOwnProperty.call(cliEngine, 'linter')) { + return cliEngine.linter.getRules() + } + + // Older versions of ESLint don't (easily) support getting a list of rules + return new Map() +} + +/** + * Given an exiting rule list and a new rule list, determines whether there + * have been changes. + * NOTE: This only accounts for presence of the rules, changes to their metadata + * are not taken into account. + * @param {Map} newRules A Map of the new rules + * @param {Map} currentRules A Map of the current rules + * @return {boolean} Whether or not there were changes + */ +export function didRulesChange(currentRules, newRules) { + return !(currentRules.size === newRules.size && + Array.from(currentRules.keys()).every(ruleId => newRules.has(ruleId))) +} diff --git a/src/worker.js b/src/worker.js index d073e11f..845ee7c7 100644 --- a/src/worker.js +++ b/src/worker.js @@ -9,64 +9,19 @@ import isConfigAtHomeRoot from './is-config-at-home-root' process.title = 'linter-eslint helper' -const fixableRules = new Set() -let sendRules = false - -/** - * Modifies the closed-over fixableRules variable when called _if_ there are - * newly-loaded fixable rules or fixable rules are removed from the set of all - * loaded rules, according to the eslint `linter` instance that is passed in. - * - * @param {Object} linter eslint 'linter' instance - * @return {void} - */ -function updateFixableRules(linter) { - if (linter === undefined) { - // ESLint < v4 doesn't support this property - return - } - - // Build a set of fixable rules based on the rules loaded in the provided linter - const currentRules = new Set() - linter.getRules().forEach((props, rule) => { - if ( - Object.prototype.hasOwnProperty.call(props, 'meta') && - Object.prototype.hasOwnProperty.call(props.meta, 'fixable') - ) { - currentRules.add(rule) - } - }) - - // Unless something has changed, we won't need to send updated set of fixableRules - sendRules = false - - // Check for new fixable rules added since the last time we sent fixableRules - const newRules = new Set(currentRules) - fixableRules.forEach(rule => newRules.delete(rule)) - if (newRules.size > 0) { - sendRules = true - } - - // Check for fixable rules that were removed since the last time we sent fixableRules - const removedRules = new Set(fixableRules) - currentRules.forEach(rule => removedRules.delete(rule)) - if (removedRules.size > 0) { - sendRules = true - } - - if (sendRules) { - // Rebuild fixableRules - fixableRules.clear() - currentRules.forEach(rule => fixableRules.add(rule)) - } -} +const rulesMetadata = new Map() +let shouldSendRules = false function lintJob({ cliEngineOptions, contents, eslint, filePath }) { const cliEngine = new eslint.CLIEngine(cliEngineOptions) const report = cliEngine.executeOnText(contents, filePath) - // Use the internal (undocumented) `linter` instance attached to the cliEngine - // to check the loaded rules (including plugin rules) and update our list of fixable rules. - updateFixableRules(cliEngine.linter) + const rules = Helpers.getRules(cliEngine) + shouldSendRules = Helpers.didRulesChange(rulesMetadata, rules) + if (shouldSendRules) { + // Rebuild rulesMetadata + rulesMetadata.clear() + rules.forEach((properties, rule) => rulesMetadata.set(rule, properties)) + } return report } @@ -113,8 +68,9 @@ module.exports = async () => { response = { messages: report.results.length ? report.results[0].messages : [] } - if (sendRules) { - response.fixableRules = Array.from(fixableRules.keys()) + if (shouldSendRules) { + // You can't emit Maps, convert to Array of Arrays to send back. + response.updatedRules = Array.from(rulesMetadata) } } else if (type === 'fix') { response = fixJob({ cliEngineOptions, contents, eslint, filePath })