From 1c2d394b51d162ef6c76ee555dee224934282d14 Mon Sep 17 00:00:00 2001 From: Landon Abney Date: Sat, 6 Jan 2018 23:50:15 -0800 Subject: [PATCH 1/8] Send full rule metadata back from the worker Refactor the worker to send the full metadata instead of just the fixable rules, allowing additional functionality to be implemented in the main package code. --- spec/helpers-spec.js | 31 +++++++++++++++++ spec/worker-helpers-spec.js | 47 ++++++++++++++++++++++++++ src/helpers.js | 39 ++++++++++++++++++++-- src/worker-helpers.js | 47 ++++++++++++++++++++++++++ src/worker.js | 66 +++++++------------------------------ 5 files changed, 173 insertions(+), 57 deletions(-) create mode 100644 spec/helpers-spec.js diff --git a/spec/helpers-spec.js b/spec/helpers-spec.js new file mode 100644 index 00000000..74d98f09 --- /dev/null +++ b/spec/helpers-spec.js @@ -0,0 +1,31 @@ +'use babel' + +import * as Helpers from '../src/helpers' + +describe('The helper module', () => { + describe('updateRules', () => { + const ruleArray = [ + ['foo', { meta: { fixable: true } }], + ['bar', { meta: {} }] + ] + + it('adds new rules', () => { + expect(Helpers.getRules()).toEqual(new Map()) + Helpers.updateRules(ruleArray) + expect(Helpers.getRules()).toEqual(new Map(ruleArray)) + }) + + it('removes old rules', () => { + Helpers.updateRules(ruleArray) + expect(Helpers.getRules()).toEqual(new Map(ruleArray)) + Helpers.updateRules([]) + expect(Helpers.getRules()).toEqual(new Map()) + }) + + it('updates the fixableRules list', () => { + expect(Helpers.getFixableRules()).toEqual([]) + Helpers.updateRules(ruleArray) + expect(Helpers.getFixableRules()).toEqual(['foo']) + }) + }) +}) diff --git a/spec/worker-helpers-spec.js b/spec/worker-helpers-spec.js index 5c961928..049c18c1 100644 --- a/spec/worker-helpers-spec.js +++ b/spec/worker-helpers-spec.js @@ -211,4 +211,51 @@ describe('Worker Helpers', () => { rimraf.sync(tempDir) }) }) + + describe('getRules', () => { + 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..c356f94c 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -8,6 +8,7 @@ import cryptoRandomString from 'crypto-random-string' // eslint-disable-next-line import/no-extraneous-dependencies, import/extensions import { Range } from 'atom' +const lintRules = new Map() const fixableRules = new Set() /** @@ -326,6 +327,39 @@ export async function processESLintMessages(messages, textEditor, showRule, work })) } +/** + * Update the list of fixable rules + */ +function updateFixableRules() { + fixableRules.clear() + lintRules.forEach((props, rule) => { + if ( + Object.prototype.hasOwnProperty.call(props, 'meta') && + Object.prototype.hasOwnProperty.call(props.meta, 'fixable') + ) { + fixableRules.add(rule) + } + }) +} + +/** + * Process the updated rules into the local Map and call further update functions + * @param {Array} updatedRules Array of Arrays of the rule and properties + */ +export function updateRules(updatedRules) { + lintRules.clear() + updatedRules.forEach(([rule, props]) => lintRules.set(rule, props)) + updateFixableRules() +} + +/** + * Return the known list of rules. + * @return {Map} The currently known rules + */ +export function getRules() { + return lintRules +} + /** * Processes the response from the lint job * @param {Object} response The raw response from the job @@ -335,9 +369,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')) { + updateRules(response.updatedRules) } return processESLintMessages(response.messages, textEditor, showRule, worker) } diff --git a/src/worker-helpers.js b/src/worker-helpers.js index 93ceb9fb..015683bf 100644 --- a/src/worker-helpers.js +++ b/src/worker-helpers.js @@ -183,3 +183,50 @@ 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) { + // 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. + * @param {[type]} newRules A map of the new rules + * @param {[type]} currentRules A Map of the current rules + * @return {boolean} Whether or not there were changes + */ +export function didRulesChange(currentRules, newRules) { + let rulesChanged = false + const currentRuleIds = new Set(currentRules.keys()) + const newRuleIds = new Set(newRules.keys()) + + // Check for new rules added since the last time we sent currentRules + const newRulesIds = new Set(newRules.keys()) + currentRuleIds.forEach(rule => newRulesIds.delete(rule)) + if (newRulesIds.size > 0) { + rulesChanged = true + } + + // Check for rules that were removed since the last time we sent currentRules + const removedRuleIds = new Set(currentRuleIds) + newRuleIds.forEach(rule => removedRuleIds.delete(rule)) + if (removedRuleIds.size > 0) { + rulesChanged = true + } + + return rulesChanged +} diff --git a/src/worker.js b/src/worker.js index d073e11f..b8bacc80 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() +const rulesMetadata = new Map() 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)) - } -} - 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) + sendRules = Helpers.didRulesChange(rulesMetadata, rules) + if (sendRules) { + // Rebuild rulesMetadata + rulesMetadata.clear() + rules.forEach((properties, rule) => rulesMetadata.set(rule, properties)) + } return report } @@ -114,7 +69,10 @@ module.exports = async () => { messages: report.results.length ? report.results[0].messages : [] } if (sendRules) { - response.fixableRules = Array.from(fixableRules.keys()) + // You can't emit Maps, convert to Array of Arrays to send back. + response.updatedRules = [] + rulesMetadata.forEach((props, rule) => + response.updatedRules.push([rule, props])) } } else if (type === 'fix') { response = fixJob({ cliEngineOptions, contents, eslint, filePath }) From 6037f5e74c836782caaf3679f662215ccf92d691 Mon Sep 17 00:00:00 2001 From: Landon Abney Date: Mon, 15 Jan 2018 15:28:54 -0800 Subject: [PATCH 2/8] Utilize URL from rule metadata The rule URL is now pulled directly from the metadata if it is available, falling back to the current method of using `eslint-rule-documentation` if the rule doesn't specify this for itself. --- spec/helpers-spec.js | 17 +++++++++++++++++ src/helpers.js | 21 ++++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/spec/helpers-spec.js b/spec/helpers-spec.js index 74d98f09..f3f1ce5d 100644 --- a/spec/helpers-spec.js +++ b/spec/helpers-spec.js @@ -28,4 +28,21 @@ describe('The helper module', () => { expect(Helpers.getFixableRules()).toEqual(['foo']) }) }) + + describe('getRuleUrl', () => { + it('works with rules that define their own URL', () => { + Helpers.updateRules([['foo', { meta: { docs: { url: 'bar' } } }]]) + expect(Helpers.getRuleUrl('foo')).toBe('bar') + }) + + it('works with rules defined in eslint-rule-documentation', () => { + const url = 'https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-duplicates.md' + expect(Helpers.getRuleUrl('import/no-duplicates')).toBe(url) + }) + + it('gives a fallback URL on how to add a rule URL', () => { + const url = 'https://github.com/jfmengels/eslint-rule-documentation/blob/master/contributing.md' + expect(Helpers.getRuleUrl('foo/bar')).toBe(url) + }) + }) }) diff --git a/src/helpers.js b/src/helpers.js index c356f94c..a163adaa 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -229,6 +229,25 @@ const generateInvalidTrace = async ({ } } +/** + * 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 + */ +export function getRuleUrl(ruleId) { + const props = lintRules.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 +} + /** * Given a raw response from ESLint, this processes the messages into a format * compatible with the Linter API. @@ -287,7 +306,7 @@ export async function processESLintMessages(messages, textEditor, showRule, work } if (ruleId) { - ret.url = ruleURI(ruleId).url + ret.url = getRuleUrl(ruleId) } let range From 57542e6049ef0c0fa50b5dbb2d9d51169f384264 Mon Sep 17 00:00:00 2001 From: Landon Abney Date: Mon, 15 Jan 2018 15:29:27 -0800 Subject: [PATCH 3/8] Use CLIEngine#getRules when available Rule gathering from the ESLint instance has been expanded to take advantage of the new `CLIEngine#getRules` method if it is available. --- spec/worker-helpers-spec.js | 7 +++++++ src/worker-helpers.js | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/spec/worker-helpers-spec.js b/spec/worker-helpers-spec.js index 049c18c1..fb40d493 100644 --- a/spec/worker-helpers-spec.js +++ b/spec/worker-helpers-spec.js @@ -213,6 +213,13 @@ describe('Worker Helpers', () => { }) 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: { diff --git a/src/worker-helpers.js b/src/worker-helpers.js index 015683bf..3ec57dad 100644 --- a/src/worker-helpers.js +++ b/src/worker-helpers.js @@ -191,6 +191,12 @@ export function getCLIEngineOptions(type, config, rules, filePath, fileDir, give * 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 From caf7d1249fa23272bc37b7377012674cdf872af4 Mon Sep 17 00:00:00 2001 From: Landon Abney Date: Mon, 15 Jan 2018 20:28:12 -0800 Subject: [PATCH 4/8] sendRules -> shouldSendRules Rename `sendRules` to `shouldSendRules` to make the purpose of the flag more clear. --- src/worker.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/worker.js b/src/worker.js index b8bacc80..0acb76ad 100644 --- a/src/worker.js +++ b/src/worker.js @@ -10,14 +10,14 @@ import isConfigAtHomeRoot from './is-config-at-home-root' process.title = 'linter-eslint helper' const rulesMetadata = new Map() -let sendRules = false +let shouldSendRules = false function lintJob({ cliEngineOptions, contents, eslint, filePath }) { const cliEngine = new eslint.CLIEngine(cliEngineOptions) const report = cliEngine.executeOnText(contents, filePath) const rules = Helpers.getRules(cliEngine) - sendRules = Helpers.didRulesChange(rulesMetadata, rules) - if (sendRules) { + shouldSendRules = Helpers.didRulesChange(rulesMetadata, rules) + if (shouldSendRules) { // Rebuild rulesMetadata rulesMetadata.clear() rules.forEach((properties, rule) => rulesMetadata.set(rule, properties)) @@ -68,7 +68,7 @@ module.exports = async () => { response = { messages: report.results.length ? report.results[0].messages : [] } - if (sendRules) { + if (shouldSendRules) { // You can't emit Maps, convert to Array of Arrays to send back. response.updatedRules = [] rulesMetadata.forEach((props, rule) => From ec95f43ff48aa7d28642bad36f7b00cfce96f09a Mon Sep 17 00:00:00 2001 From: Landon Abney Date: Mon, 15 Jan 2018 20:35:14 -0800 Subject: [PATCH 5/8] Simplify the logic behind didRulesChange Use a much simpler logic to determine whether the rule `Map`s are equal or not. Also fixes up the docblock a bit. --- src/worker-helpers.js | 27 ++++++--------------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/src/worker-helpers.js b/src/worker-helpers.js index 3ec57dad..bdcfd47e 100644 --- a/src/worker-helpers.js +++ b/src/worker-helpers.js @@ -211,28 +211,13 @@ export function getRules(cliEngine) { /** * Given an exiting rule list and a new rule list, determines whether there * have been changes. - * @param {[type]} newRules A map of the new rules - * @param {[type]} currentRules A Map of the current rules + * 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) { - let rulesChanged = false - const currentRuleIds = new Set(currentRules.keys()) - const newRuleIds = new Set(newRules.keys()) - - // Check for new rules added since the last time we sent currentRules - const newRulesIds = new Set(newRules.keys()) - currentRuleIds.forEach(rule => newRulesIds.delete(rule)) - if (newRulesIds.size > 0) { - rulesChanged = true - } - - // Check for rules that were removed since the last time we sent currentRules - const removedRuleIds = new Set(currentRuleIds) - newRuleIds.forEach(rule => removedRuleIds.delete(rule)) - if (removedRuleIds.size > 0) { - rulesChanged = true - } - - return rulesChanged + return !(currentRules.size === newRules.size && + Array.from(currentRules.keys()).every(ruleId => newRules.has(ruleId))) } From 6096bd4fdec3205b1e41b2da052b38dca2347f36 Mon Sep 17 00:00:00 2001 From: Landon Abney Date: Mon, 15 Jan 2018 20:35:53 -0800 Subject: [PATCH 6/8] Use Array.from on the rule Map Apparently `Array.from` is quite happy taking a `Map` object directly. --- src/worker.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/worker.js b/src/worker.js index 0acb76ad..845ee7c7 100644 --- a/src/worker.js +++ b/src/worker.js @@ -70,9 +70,7 @@ module.exports = async () => { } if (shouldSendRules) { // You can't emit Maps, convert to Array of Arrays to send back. - response.updatedRules = [] - rulesMetadata.forEach((props, rule) => - response.updatedRules.push([rule, props])) + response.updatedRules = Array.from(rulesMetadata) } } else if (type === 'fix') { response = fixJob({ cliEngineOptions, contents, eslint, filePath }) From 03af716da6ab62e2fd1ef0d30608abb44c3c0336 Mon Sep 17 00:00:00 2001 From: Landon Abney Date: Mon, 15 Jan 2018 21:36:38 -0800 Subject: [PATCH 7/8] Move rule related logic to a class Pull all the logic related to storing the list of known rules and accessing it out into a class of its own. --- spec/helpers-spec.js | 48 -------------------------------- spec/rules-spec.js | 54 ++++++++++++++++++++++++++++++++++++ src/helpers.js | 66 +++----------------------------------------- src/main.js | 2 +- src/rules.js | 66 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 125 insertions(+), 111 deletions(-) delete mode 100644 spec/helpers-spec.js create mode 100644 spec/rules-spec.js create mode 100644 src/rules.js diff --git a/spec/helpers-spec.js b/spec/helpers-spec.js deleted file mode 100644 index f3f1ce5d..00000000 --- a/spec/helpers-spec.js +++ /dev/null @@ -1,48 +0,0 @@ -'use babel' - -import * as Helpers from '../src/helpers' - -describe('The helper module', () => { - describe('updateRules', () => { - const ruleArray = [ - ['foo', { meta: { fixable: true } }], - ['bar', { meta: {} }] - ] - - it('adds new rules', () => { - expect(Helpers.getRules()).toEqual(new Map()) - Helpers.updateRules(ruleArray) - expect(Helpers.getRules()).toEqual(new Map(ruleArray)) - }) - - it('removes old rules', () => { - Helpers.updateRules(ruleArray) - expect(Helpers.getRules()).toEqual(new Map(ruleArray)) - Helpers.updateRules([]) - expect(Helpers.getRules()).toEqual(new Map()) - }) - - it('updates the fixableRules list', () => { - expect(Helpers.getFixableRules()).toEqual([]) - Helpers.updateRules(ruleArray) - expect(Helpers.getFixableRules()).toEqual(['foo']) - }) - }) - - describe('getRuleUrl', () => { - it('works with rules that define their own URL', () => { - Helpers.updateRules([['foo', { meta: { docs: { url: 'bar' } } }]]) - expect(Helpers.getRuleUrl('foo')).toBe('bar') - }) - - it('works with rules defined in eslint-rule-documentation', () => { - const url = 'https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-duplicates.md' - expect(Helpers.getRuleUrl('import/no-duplicates')).toBe(url) - }) - - it('gives a fallback URL on how to add a rule URL', () => { - const url = 'https://github.com/jfmengels/eslint-rule-documentation/blob/master/contributing.md' - expect(Helpers.getRuleUrl('foo/bar')).toBe(url) - }) - }) -}) diff --git a/spec/rules-spec.js b/spec/rules-spec.js new file mode 100644 index 00000000..a8951c66 --- /dev/null +++ b/spec/rules-spec.js @@ -0,0 +1,54 @@ +'use babel' + +import Rules from '../src/rules' + +describe('The Rules class', () => { + describe('updateRules', () => { + const ruleArray = [ + ['foo', { meta: { fixable: true } }], + ['bar', { meta: {} }] + ] + + it('adds new rules', () => { + const rules = new Rules() + expect(rules.getRules()).toEqual(new Map()) + rules.updateRules(ruleArray) + expect(rules.getRules()).toEqual(new Map(ruleArray)) + }) + + it('removes old rules', () => { + const rules = new Rules() + rules.updateRules(ruleArray) + expect(rules.getRules()).toEqual(new Map(ruleArray)) + rules.updateRules([]) + expect(rules.getRules()).toEqual(new Map()) + }) + + it('updates the fixableRules list', () => { + const rules = new Rules() + expect(rules.getFixableRules()).toEqual([]) + rules.updateRules(ruleArray) + expect(rules.getFixableRules()).toEqual(['foo']) + }) + }) + + describe('getRuleUrl', () => { + it('works with rules that define their own URL', () => { + const rules = new Rules() + rules.updateRules([['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/src/helpers.js b/src/helpers.js index a163adaa..0eed0082 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -1,15 +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 lintRules = new Map() -const fixableRules = new Set() +export const rules = new Rules() /** * Start the worker process if it hasn't already been started @@ -73,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])) { @@ -229,25 +223,6 @@ const generateInvalidTrace = async ({ } } -/** - * 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 - */ -export function getRuleUrl(ruleId) { - const props = lintRules.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 -} - /** * Given a raw response from ESLint, this processes the messages into a format * compatible with the Linter API. @@ -306,7 +281,7 @@ export async function processESLintMessages(messages, textEditor, showRule, work } if (ruleId) { - ret.url = getRuleUrl(ruleId) + ret.url = rules.getRuleUrl(ruleId) } let range @@ -346,39 +321,6 @@ export async function processESLintMessages(messages, textEditor, showRule, work })) } -/** - * Update the list of fixable rules - */ -function updateFixableRules() { - fixableRules.clear() - lintRules.forEach((props, rule) => { - if ( - Object.prototype.hasOwnProperty.call(props, 'meta') && - Object.prototype.hasOwnProperty.call(props.meta, 'fixable') - ) { - fixableRules.add(rule) - } - }) -} - -/** - * Process the updated rules into the local Map and call further update functions - * @param {Array} updatedRules Array of Arrays of the rule and properties - */ -export function updateRules(updatedRules) { - lintRules.clear() - updatedRules.forEach(([rule, props]) => lintRules.set(rule, props)) - updateFixableRules() -} - -/** - * Return the known list of rules. - * @return {Map} The currently known rules - */ -export function getRules() { - return lintRules -} - /** * Processes the response from the lint job * @param {Object} response The raw response from the job @@ -389,7 +331,7 @@ export function getRules() { */ export async function processJobResponse(response, textEditor, showRule, worker) { if (Object.prototype.hasOwnProperty.call(response, 'updatedRules')) { - updateRules(response.updatedRules) + rules.updateRules(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..13be709e --- /dev/null +++ b/src/rules.js @@ -0,0 +1,66 @@ +import ruleURI from 'eslint-rule-documentation' + +/** + * 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) { + this.rules = new Map() + if (newRules) { + this.updateRules(newRules) + } + } + + /** + * Process the updated rules into the local Map and call further update functions + * @param {Array} updatedRules Array of Arrays of the rule and properties + */ + updateRules(updatedRules) { + this.rules.clear() + updatedRules.forEach(([rule, props]) => this.rules.set(rule, props)) + } + + /** + * [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) + } +} From deb5c317ad282b1ac8928a8cb69f4cfea5d26aab Mon Sep 17 00:00:00 2001 From: Landon Abney Date: Wed, 17 Jan 2018 21:35:40 -0800 Subject: [PATCH 8/8] Validate rules before replacing the current set Check that the given Array contains Arrays that "look" like the exected format: [String, Object]. Also renames `updatedRules` to `replaceRules` since it is now taking advantage of the Map constructor to parse through the Array. --- spec/rules-spec.js | 41 +++++++++++++++++++++++++++++++++++------ src/helpers.js | 2 +- src/rules.js | 42 ++++++++++++++++++++++++++++++++---------- 3 files changed, 68 insertions(+), 17 deletions(-) diff --git a/spec/rules-spec.js b/spec/rules-spec.js index a8951c66..8808ad8a 100644 --- a/spec/rules-spec.js +++ b/spec/rules-spec.js @@ -3,7 +3,7 @@ import Rules from '../src/rules' describe('The Rules class', () => { - describe('updateRules', () => { + describe('replaceRules', () => { const ruleArray = [ ['foo', { meta: { fixable: true } }], ['bar', { meta: {} }] @@ -12,30 +12,59 @@ describe('The Rules class', () => { it('adds new rules', () => { const rules = new Rules() expect(rules.getRules()).toEqual(new Map()) - rules.updateRules(ruleArray) + rules.replaceRules(ruleArray) expect(rules.getRules()).toEqual(new Map(ruleArray)) }) it('removes old rules', () => { const rules = new Rules() - rules.updateRules(ruleArray) + rules.replaceRules(ruleArray) expect(rules.getRules()).toEqual(new Map(ruleArray)) - rules.updateRules([]) + rules.replaceRules([]) expect(rules.getRules()).toEqual(new Map()) }) it('updates the fixableRules list', () => { const rules = new Rules() expect(rules.getFixableRules()).toEqual([]) - rules.updateRules(ruleArray) + 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.updateRules([['foo', { meta: { docs: { url: 'bar' } } }]]) + rules.replaceRules([['foo', { meta: { docs: { url: 'bar' } } }]]) expect(rules.getRuleUrl('foo')).toBe('bar') }) diff --git a/src/helpers.js b/src/helpers.js index 0eed0082..8cf91292 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -331,7 +331,7 @@ export async function processESLintMessages(messages, textEditor, showRule, work */ export async function processJobResponse(response, textEditor, showRule, worker) { if (Object.prototype.hasOwnProperty.call(response, 'updatedRules')) { - rules.updateRules(response.updatedRules) + rules.replaceRules(response.updatedRules) } return processESLintMessages(response.messages, textEditor, showRule, worker) } diff --git a/src/rules.js b/src/rules.js index 13be709e..ab685418 100644 --- a/src/rules.js +++ b/src/rules.js @@ -1,5 +1,10 @@ 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 */ @@ -9,19 +14,20 @@ export default class Rules { * @param {Array} newRules Array of Arrays of the rule and properties */ constructor(newRules) { - this.rules = new Map() - if (newRules) { - this.updateRules(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} updatedRules Array of Arrays of the rule and properties + * @param {Array} newRules Array of Arrays of the rule and properties */ - updateRules(updatedRules) { - this.rules.clear() - updatedRules.forEach(([rule, props]) => this.rules.set(rule, props)) + replaceRules(newRules) { + Rules[validateRulesArray](newRules) + this[rules] = new Map(newRules) } /** @@ -29,7 +35,7 @@ export default class Rules { * @return {Array} The ruleIds of the currently known fixable rules */ getFixableRules() { - return Array.from(this.rules).reduce((fixable, [rule, props]) => { + return Array.from(this[rules]).reduce((fixable, [rule, props]) => { if (props.meta && props.meta.fixable) { return [...fixable, rule] } @@ -45,7 +51,7 @@ export default class Rules { * @return {String} URL of the rule documentation */ getRuleUrl(ruleId) { - const props = this.rules.get(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 @@ -61,6 +67,22 @@ export default class Rules { * @return {Map} The currently known rules */ getRules() { - return new Map(this.rules) + 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!') + } } }