From f03fba50fd23cd4868b634d781e39c7c3facf564 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Sat, 20 Feb 2016 20:20:27 -0500 Subject: [PATCH 01/14] first draft of no-deprecated test + empty rule --- src/rules/no-deprecated.js | 9 +++++++++ tests/files/deprecated.js | 6 ++++++ tests/src/rules/no-deprecated.js | 20 ++++++++++++++++++++ 3 files changed, 35 insertions(+) create mode 100644 src/rules/no-deprecated.js create mode 100644 tests/files/deprecated.js create mode 100644 tests/src/rules/no-deprecated.js diff --git a/src/rules/no-deprecated.js b/src/rules/no-deprecated.js new file mode 100644 index 0000000000..682abee9f0 --- /dev/null +++ b/src/rules/no-deprecated.js @@ -0,0 +1,9 @@ +import Exports from '../core/getExports' + +module.exports = function (context) { + return { + 'ImportDeclaration': function (n) { + // check specifiers + }, + } +} diff --git a/tests/files/deprecated.js b/tests/files/deprecated.js new file mode 100644 index 0000000000..eeaf1f2610 --- /dev/null +++ b/tests/files/deprecated.js @@ -0,0 +1,6 @@ +/** + * this function is terrible + * @deprecated please use 'x' instead. + * @return null + */ +export function fn() { return null } diff --git a/tests/src/rules/no-deprecated.js b/tests/src/rules/no-deprecated.js new file mode 100644 index 0000000000..aea37fd697 --- /dev/null +++ b/tests/src/rules/no-deprecated.js @@ -0,0 +1,20 @@ +import * as path from 'path' +import { test } from '../utils' + +import { RuleTester } from 'eslint' + +const ruleTester = new RuleTester() + , rule = require('rules/no-deprecated') + +ruleTester.run('no-deprecated', rule, { + valid: [ + test({ code: "import { x } from './fake' " }), + test({ code: "import bar from './bar'" }), + ], + invalid: [ + test({ + code: "import { fn } from './deprecated'", + errors: ["Deprecated: please use 'x' instead."], + }), + ], +}) From edab24b48c6ad03b29bd78ca97d258608c578d3a Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Mon, 22 Feb 2016 06:00:30 -0500 Subject: [PATCH 02/14] simplified export collection --- src/core/getExports.js | 106 +++++++++++++++++------------------------ 1 file changed, 44 insertions(+), 62 deletions(-) diff --git a/src/core/getExports.js b/src/core/getExports.js index c40f500c5c..6e047a3fe4 100644 --- a/src/core/getExports.js +++ b/src/core/getExports.js @@ -81,9 +81,50 @@ export default class ExportMap { } ast.body.forEach(function (n) { - m.captureDefault(n) - m.captureAll(n, path) - m.captureNamedDeclaration(n, path) + if (n.type === 'ExportDefaultDeclaration') { + m.named.add('default') + return + } + + if (n.type === 'ExportAllDeclaration') { + let remoteMap = m.resolveReExport(n, path) + if (remoteMap == null) return + + remoteMap.named.forEach((name) => { m.named.add(name) }) + return + } + + if (n.type === 'ExportNamedDeclaration'){ + + // capture declaration + if (n.declaration != null) { + switch (n.declaration.type) { + case 'FunctionDeclaration': + case 'ClassDeclaration': + case 'TypeAlias': // flowtype with babel-eslint parser + m.named.add(n.declaration.id.name) + break + case 'VariableDeclaration': + n.declaration.declarations.forEach((d) => + recursivePatternCapture(d.id, id => m.named.add(id.name))) + break + } + } + + // capture specifiers + let remoteMap + if (n.source) remoteMap = m.resolveReExport(n, path) + + n.specifiers.forEach(function (s) { + if (s.type === 'ExportDefaultSpecifier') { + // don't add it if it is not present in the exported module + if (!remoteMap || !remoteMap.hasDefault) return + } + + m.named.add(s.exported.name) + }) + } + }) return m @@ -96,65 +137,6 @@ export default class ExportMap { return ExportMap.for(remotePath, this.context) } - captureDefault(n) { - if (n.type !== 'ExportDefaultDeclaration') return - this.named.add('default') - } - - /** - * capture all named exports from remote module. - * - * returns null if this node wasn't an ExportAllDeclaration - * returns false if it was not resolved - * returns true if it was resolved + parsed - * - * @param {node} n - * @param {string} path - the path of the module currently parsing - * @return {boolean?} - */ - captureAll(n, path) { - if (n.type !== 'ExportAllDeclaration') return null - - var remoteMap = this.resolveReExport(n, path) - if (remoteMap == null) return false - - remoteMap.named.forEach(function (name) { this.named.add(name) }.bind(this)) - - return true - } - - captureNamedDeclaration(n, path) { - if (n.type !== 'ExportNamedDeclaration') return - - // capture declaration - if (n.declaration != null) { - switch (n.declaration.type) { - case 'FunctionDeclaration': - case 'ClassDeclaration': - case 'TypeAlias': // flowtype with babel-eslint parser - this.named.add(n.declaration.id.name) - break - case 'VariableDeclaration': - n.declaration.declarations.forEach((d) => - recursivePatternCapture(d.id, id => this.named.add(id.name))) - break - } - } - - // capture specifiers - let remoteMap - if (n.source) remoteMap = this.resolveReExport(n, path) - - n.specifiers.forEach(function (s) { - if (s.type === 'ExportDefaultSpecifier') { - // don't add it if it is not present in the exported module - if (!remoteMap || !remoteMap.hasDefault) return - } - - this.named.add(s.exported.name) - }.bind(this)) - } - reportErrors(context, declaration) { context.report({ node: declaration.source, From 6419231daa5cf96a273ad0ea7b80e97d5d394445 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Mon, 22 Feb 2016 06:37:07 -0500 Subject: [PATCH 03/14] ExportMap exports JSDoc metadata, if found. --- package.json | 1 + src/core/getExports.js | 34 +++++++++++----- src/rules/export.js | 2 +- tests/files/deprecated.js | 11 ++++++ tests/src/core/getExports.js | 75 ++++++++++++++++++++++++++++-------- 5 files changed, 96 insertions(+), 27 deletions(-) diff --git a/package.json b/package.json index ded1d716b8..ae63924278 100644 --- a/package.json +++ b/package.json @@ -61,6 +61,7 @@ }, "dependencies": { "babel-runtime": "6.5.0", + "doctrine": "1.2.0", "eslint-import-resolver-node": "^0.1.0" }, "greenkeeper": { diff --git a/src/core/getExports.js b/src/core/getExports.js index 6e047a3fe4..096b33cd10 100644 --- a/src/core/getExports.js +++ b/src/core/getExports.js @@ -1,6 +1,7 @@ import * as fs from 'fs' import { createHash } from 'crypto' +import * as doctrine from 'doctrine' import parse from './parse' import resolve from './resolve' @@ -12,7 +13,7 @@ const exportCaches = new Map() export default class ExportMap { constructor(context) { this.context = context - this.named = new Set() + this.named = new Map() this.errors = [] } @@ -81,32 +82,31 @@ export default class ExportMap { } ast.body.forEach(function (n) { + if (n.type === 'ExportDefaultDeclaration') { - m.named.add('default') + m.named.set('default', captureMetadata(n)) return } if (n.type === 'ExportAllDeclaration') { let remoteMap = m.resolveReExport(n, path) if (remoteMap == null) return - - remoteMap.named.forEach((name) => { m.named.add(name) }) + remoteMap.named.forEach((value, name) => { m.named.set(name, value) }) return } if (n.type === 'ExportNamedDeclaration'){ - // capture declaration if (n.declaration != null) { switch (n.declaration.type) { case 'FunctionDeclaration': case 'ClassDeclaration': case 'TypeAlias': // flowtype with babel-eslint parser - m.named.add(n.declaration.id.name) + m.named.set(n.declaration.id.name, captureMetadata(n)) break case 'VariableDeclaration': n.declaration.declarations.forEach((d) => - recursivePatternCapture(d.id, id => m.named.add(id.name))) + recursivePatternCapture(d.id, id => m.named.set(id.name, null))) break } } @@ -120,8 +120,7 @@ export default class ExportMap { // don't add it if it is not present in the exported module if (!remoteMap || !remoteMap.hasDefault) return } - - m.named.add(s.exported.name) + m.named.set(s.exported.name, null) }) } @@ -148,6 +147,23 @@ export default class ExportMap { } } +function captureMetadata(n) { + const metadata = {} + // capture XSDoc + if (n.leadingComments) { + n.leadingComments.forEach(comment => { + // skip non-block comments + if (comment.value.slice(0, 4) !== "*\n *") return + try { + metadata.doc = doctrine.parse(comment.value, { unwrap: true }) + } catch (err) { + /* don't care, for now? maybe add to `errors?` */ + } + }) + } + + return metadata +} /** * Traverse a patter/identifier node, calling 'callback' diff --git a/src/rules/export.js b/src/rules/export.js index 25bfdbe32d..a7ee5a991e 100644 --- a/src/rules/export.js +++ b/src/rules/export.js @@ -54,7 +54,7 @@ module.exports = function (context) { `No named exports found in module '${node.source.value}'.`) } - for (let name of remoteExports.named) { + for (let name of remoteExports.named.keys()) { addNamed(name, node) } }, diff --git a/tests/files/deprecated.js b/tests/files/deprecated.js index eeaf1f2610..7e5871facf 100644 --- a/tests/files/deprecated.js +++ b/tests/files/deprecated.js @@ -1,6 +1,17 @@ +// some line comment /** * this function is terrible * @deprecated please use 'x' instead. * @return null */ +// another line comment +// with two lines export function fn() { return null } + +/** + * so terrible + * @deprecated this is awful, use NotAsBadClass. + */ +export default class TerribleClass { + +} diff --git a/tests/src/core/getExports.js b/tests/src/core/getExports.js index 940f406257..4b0219106a 100644 --- a/tests/src/core/getExports.js +++ b/tests/src/core/getExports.js @@ -5,6 +5,14 @@ import * as fs from 'fs' import { getFilename } from '../utils' +const fakeDefaultContext = { + parserPath: 'espree', + parserOptions: { + sourceType: 'module', + attachComment: true, + }, +} + describe('getExports', function () { const fakeContext = { getFilename: getFilename, @@ -87,22 +95,55 @@ describe('getExports', function () { expect(imports.named.has('Bar')).to.be.true }) - // it('finds exports for an ES7 module with proper parse options', function () { - // var imports = ExportMap.parse( - // getFilename('jsx/FooES7.js'), - // { - // parserPath: 'espree', - // parserOptions: { - // ecmaVersion: 7, - // ecmaFeatures: { jsx: true }, - // }, - // } - // ) - - // expect(imports).to.exist - // expect(imports.errors).to.be.empty - // expect(imports).to.have.property('hasDefault', true) - // expect(imports.named.has('Bar')).to.be.true - // }) + context('deprecation metadata', function () { + + function jsdocTests(parseContext) { + let imports + before('parse file', function () { + imports = ExportMap.parse( + getFilename('deprecated.js'), parseContext) + + // sanity checks + expect(imports.errors).to.be.empty + }) + + it('works with named imports.', function () { + expect(imports.named.has('fn')).to.be.true + + expect(imports.named.get('fn')) + .to.have.deep.property('doc.tags[0].title', 'deprecated') + expect(imports.named.get('fn')) + .to.have.deep.property('doc.tags[0].description', "please use 'x' instead.") + }) + + it('works with default imports.', function () { + expect(imports.named.has('default')).to.be.true + const importMeta = imports.named.get('default') + + expect(importMeta).to.have.deep.property('doc.tags[0].title', 'deprecated') + expect(importMeta).to.have.deep.property('doc.tags[0].description', 'this is awful, use NotAsBadClass.') + }) + } + + context('default parser', function () { + jsdocTests({ + parserPath: 'espree', + parserOptions: { + sourceType: 'module', + attachComment: true, + }, + }) + }) + + context('babel-eslint', function () { + jsdocTests({ + parserPath: 'babel-eslint', + parserOptions: { + sourceType: 'module', + attachComment: true, + }, + }) + }) + }) }) From 6b42484497efa72441c9beb26df86a3515a2a955 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Mon, 22 Feb 2016 06:42:14 -0500 Subject: [PATCH 04/14] export no-deprecated rule --- src/index.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/index.js b/src/index.js index 5067399b81..96877e706d 100644 --- a/src/index.js +++ b/src/index.js @@ -11,6 +11,9 @@ export const rules = { 'no-amd': require('./rules/no-amd'), 'no-duplicates': require('./rules/no-duplicates'), 'imports-first': require('./rules/imports-first'), + + // metadata-based + 'no-deprecated': require('./rules/no-deprecated'), } export const configs = { From c68d44a64234163e2ee5255c23ad67362f28ce01 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Mon, 22 Feb 2016 07:00:41 -0500 Subject: [PATCH 05/14] first draft of no-deprecated! flags explicit default + named imports for being deprecated. --- src/core/parse.js | 3 +++ src/rules/no-deprecated.js | 37 +++++++++++++++++++++++++++++--- tests/files/deprecated.js | 8 +++++++ tests/src/core/getExports.js | 8 ------- tests/src/rules/no-deprecated.js | 7 ++++++ 5 files changed, 52 insertions(+), 11 deletions(-) diff --git a/src/core/parse.js b/src/core/parse.js index 1d9fa60d57..ef7d12903f 100644 --- a/src/core/parse.js +++ b/src/core/parse.js @@ -12,6 +12,9 @@ export default function (p, context) { parserOptions = Object.assign({}, parserOptions) parserOptions.ecmaFeatures = Object.assign({}, parserOptions.ecmaFeatures) + // always attach comments + parserOptions.attachComment = true + // require the parser relative to the main module (i.e., ESLint) const parser = requireParser(parserPath) diff --git a/src/rules/no-deprecated.js b/src/rules/no-deprecated.js index 682abee9f0..dd860f12db 100644 --- a/src/rules/no-deprecated.js +++ b/src/rules/no-deprecated.js @@ -1,9 +1,40 @@ import Exports from '../core/getExports' module.exports = function (context) { + function checkSpecifiers(node) { + if (node.source == null) return // local export, ignore + + const imports = Exports.get(node.source.value, context) + if (imports == null) return + + if (imports.errors.length) { + imports.reportErrors(context, node) + return + } + + node.specifiers.forEach(function (im) { + let imported + switch (im.type) { + case 'ImportDefaultSpecifier': imported = 'default'; break + case 'ImportSpecifier': imported = im.imported.name; break + default: return // can't handle this one + } + + // unknown thing can't be deprecated + if (!imports.named.has(imported)) return + + const metadata = imports.named.get(imported) + if (!metadata || !metadata.doc) return + + let deprecation + if (metadata.doc.tags.some(t => t.title === 'deprecated' && (deprecation = t))) { + context.report(im, + 'Deprecated' + (deprecation.description ? ': ' + deprecation.description : '.')) + } + }) + } + return { - 'ImportDeclaration': function (n) { - // check specifiers - }, + 'ImportDeclaration': checkSpecifiers, } } diff --git a/tests/files/deprecated.js b/tests/files/deprecated.js index 7e5871facf..f7b071a25c 100644 --- a/tests/files/deprecated.js +++ b/tests/files/deprecated.js @@ -15,3 +15,11 @@ export function fn() { return null } export default class TerribleClass { } + +/** + * this one is fine + * @return {String} - great! + */ +export function fine() { return "great!" } + +export function _undocumented() { return "sneaky!" } diff --git a/tests/src/core/getExports.js b/tests/src/core/getExports.js index 4b0219106a..c35f633f1b 100644 --- a/tests/src/core/getExports.js +++ b/tests/src/core/getExports.js @@ -5,14 +5,6 @@ import * as fs from 'fs' import { getFilename } from '../utils' -const fakeDefaultContext = { - parserPath: 'espree', - parserOptions: { - sourceType: 'module', - attachComment: true, - }, -} - describe('getExports', function () { const fakeContext = { getFilename: getFilename, diff --git a/tests/src/rules/no-deprecated.js b/tests/src/rules/no-deprecated.js index aea37fd697..2722a6abeb 100644 --- a/tests/src/rules/no-deprecated.js +++ b/tests/src/rules/no-deprecated.js @@ -10,11 +10,18 @@ ruleTester.run('no-deprecated', rule, { valid: [ test({ code: "import { x } from './fake' " }), test({ code: "import bar from './bar'" }), + + test({ code: "import { fine } from './deprecated'" }), + test({ code: "import { _undocumented } from './deprecated'" }), ], invalid: [ test({ code: "import { fn } from './deprecated'", errors: ["Deprecated: please use 'x' instead."], }), + test({ + code: "import TerribleClass from './deprecated'", + errors: ["Deprecated: this is awful, use NotAsBadClass."], + }), ], }) From 4283e373f353e1e54007833733448623932bb378 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Mon, 22 Feb 2016 07:09:32 -0500 Subject: [PATCH 06/14] rudimentary constant deprecation capture --- src/core/getExports.js | 2 +- tests/files/deprecated.js | 7 +++++++ tests/src/core/getExports.js | 10 ++++++++++ tests/src/rules/no-deprecated.js | 4 ++++ 4 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/core/getExports.js b/src/core/getExports.js index 096b33cd10..b29626ce18 100644 --- a/src/core/getExports.js +++ b/src/core/getExports.js @@ -106,7 +106,7 @@ export default class ExportMap { break case 'VariableDeclaration': n.declaration.declarations.forEach((d) => - recursivePatternCapture(d.id, id => m.named.set(id.name, null))) + recursivePatternCapture(d.id, id => m.named.set(id.name, captureMetadata(n)))) break } } diff --git a/tests/files/deprecated.js b/tests/files/deprecated.js index f7b071a25c..dd49070012 100644 --- a/tests/files/deprecated.js +++ b/tests/files/deprecated.js @@ -16,6 +16,13 @@ export default class TerribleClass { } +/** + * some flux action type maybe + * @deprecated please stop sending/handling this action type. + * @type {String} + */ +export const MY_TERRIBLE_ACTION = "ugh" + /** * this one is fine * @return {String} - great! diff --git a/tests/src/core/getExports.js b/tests/src/core/getExports.js index c35f633f1b..77deceac21 100644 --- a/tests/src/core/getExports.js +++ b/tests/src/core/getExports.js @@ -115,6 +115,16 @@ describe('getExports', function () { expect(importMeta).to.have.deep.property('doc.tags[0].title', 'deprecated') expect(importMeta).to.have.deep.property('doc.tags[0].description', 'this is awful, use NotAsBadClass.') }) + + it('works with variables.', function () { + expect(imports.named.has('MY_TERRIBLE_ACTION')).to.be.true + const importMeta = imports.named.get('MY_TERRIBLE_ACTION') + + expect(importMeta).to.have.deep.property( + 'doc.tags[0].title', 'deprecated') + expect(importMeta).to.have.deep.property( + 'doc.tags[0].description', 'please stop sending/handling this action type.') + }) } context('default parser', function () { diff --git a/tests/src/rules/no-deprecated.js b/tests/src/rules/no-deprecated.js index 2722a6abeb..0788192abc 100644 --- a/tests/src/rules/no-deprecated.js +++ b/tests/src/rules/no-deprecated.js @@ -23,5 +23,9 @@ ruleTester.run('no-deprecated', rule, { code: "import TerribleClass from './deprecated'", errors: ["Deprecated: this is awful, use NotAsBadClass."], }), + test({ + code: "import { MY_TERRIBLE_ACTION } from './deprecated'", + errors: ["Deprecated: please stop sending/handling this action type."], + }), ], }) From 4a428401f991912e4e8ed5bc6deca01c2e12f9fd Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Mon, 22 Feb 2016 07:19:55 -0500 Subject: [PATCH 07/14] JSDoc for comma-delimited constant export --- src/core/getExports.js | 21 +++++++++++++++------ tests/files/deprecated.js | 17 +++++++++++++++++ tests/src/core/getExports.js | 30 ++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 6 deletions(-) diff --git a/src/core/getExports.js b/src/core/getExports.js index b29626ce18..38e7cdeeb9 100644 --- a/src/core/getExports.js +++ b/src/core/getExports.js @@ -106,7 +106,7 @@ export default class ExportMap { break case 'VariableDeclaration': n.declaration.declarations.forEach((d) => - recursivePatternCapture(d.id, id => m.named.set(id.name, captureMetadata(n)))) + recursivePatternCapture(d.id, id => m.named.set(id.name, captureMetadata(d, n)))) break } } @@ -147,10 +147,19 @@ export default class ExportMap { } } -function captureMetadata(n) { +/** + * parse JSDoc from the first node that has leading comments + * @param {...[type]} nodes [description] + * @return {[type]} [description] + */ +function captureMetadata(...nodes) { const metadata = {} - // capture XSDoc - if (n.leadingComments) { + + // 'some' short-circuits on first 'true' + nodes.some(n => { + if (!n.leadingComments) return false + + // capture XSDoc n.leadingComments.forEach(comment => { // skip non-block comments if (comment.value.slice(0, 4) !== "*\n *") return @@ -160,8 +169,8 @@ function captureMetadata(n) { /* don't care, for now? maybe add to `errors?` */ } }) - } - + return true + }) return metadata } diff --git a/tests/files/deprecated.js b/tests/files/deprecated.js index dd49070012..10e81dc912 100644 --- a/tests/files/deprecated.js +++ b/tests/files/deprecated.js @@ -23,6 +23,23 @@ export default class TerribleClass { */ export const MY_TERRIBLE_ACTION = "ugh" +/** + * @deprecated this chain is awful + * @type {String} + */ +export const CHAIN_A = "a" +/** + * @deprecated so awful + * @type {String} + */ + , CHAIN_B = "b" + +/** + * @deprecated still terrible + * @type {String} + */ + , CHAIN_C = "C" + /** * this one is fine * @return {String} - great! diff --git a/tests/src/core/getExports.js b/tests/src/core/getExports.js index 77deceac21..df82d4c8ea 100644 --- a/tests/src/core/getExports.js +++ b/tests/src/core/getExports.js @@ -125,6 +125,36 @@ describe('getExports', function () { expect(importMeta).to.have.deep.property( 'doc.tags[0].description', 'please stop sending/handling this action type.') }) + + context('multi-line variables', function () { + it('works for the first one', function () { + expect(imports.named.has('CHAIN_A')).to.be.true + const importMeta = imports.named.get('CHAIN_A') + + expect(importMeta).to.have.deep.property( + 'doc.tags[0].title', 'deprecated') + expect(importMeta).to.have.deep.property( + 'doc.tags[0].description', 'this chain is awful') + }) + it('works for the second one', function () { + expect(imports.named.has('CHAIN_B')).to.be.true + const importMeta = imports.named.get('CHAIN_B') + + expect(importMeta).to.have.deep.property( + 'doc.tags[0].title', 'deprecated') + expect(importMeta).to.have.deep.property( + 'doc.tags[0].description', 'so awful') + }) + it('works for the third one, etc.', function () { + expect(imports.named.has('CHAIN_C')).to.be.true + const importMeta = imports.named.get('CHAIN_C') + + expect(importMeta).to.have.deep.property( + 'doc.tags[0].title', 'deprecated') + expect(importMeta).to.have.deep.property( + 'doc.tags[0].description', 'still terrible') + }) + }) } context('default parser', function () { From 702d4d49cb3638752431288fd32df80dc24760da Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Mon, 22 Feb 2016 07:27:36 -0500 Subject: [PATCH 08/14] stage 0 deprecated documentation --- docs/rules/no-deprecated.md | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 docs/rules/no-deprecated.md diff --git a/docs/rules/no-deprecated.md b/docs/rules/no-deprecated.md new file mode 100644 index 0000000000..6702a7beac --- /dev/null +++ b/docs/rules/no-deprecated.md @@ -0,0 +1,33 @@ +# no-deprecated + +**Stage: 0** + +**NOTE**: this rule is currently a work in progress. There may be "breaking" changes: most likely, additional cases that are flagged. + +Reports use of a deprecated name, as indicated by a JSDoc block with a `@deprecated` +tag, i.e. + +```js +// @file: ./answer.js + +/** + * this is what you get when you trust a mouse talk show + * @deprecated need to restart the experiment + * @returns {Number} nonsense + */ +export function multiply(six, nine) { + return 42 +} +``` + +will report as such: + +```js +import { multiply } from './answer' // Deprecated: need to restart the experiment +``` + +### Worklist + +- [x] report explicit imports on the import node +- [ ] support namespaces +- [ ] report explicit imports at reference time (at the identifier) similar to namespace From 899c7c0538393cd8ed6421c74854b137dae031fd Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Mon, 22 Feb 2016 07:36:00 -0500 Subject: [PATCH 09/14] config stage-0: holding pen for rules in progress --- config/stage-0.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 config/stage-0.js diff --git a/config/stage-0.js b/config/stage-0.js new file mode 100644 index 0000000000..1a77784861 --- /dev/null +++ b/config/stage-0.js @@ -0,0 +1,12 @@ +/** + * Rules in progress. + * + * Do not expect these to adhere to semver across releases. + * @type {Object} + */ +module.exports = { + plugins: ['import'], + rules: { + 'import/no-deprecated': 1, + } +} From 1285720c4c45abcc4147211579162ed388a9e2b3 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Mon, 22 Feb 2016 08:00:35 -0500 Subject: [PATCH 10/14] restructured to allow successful install from GitHub. --- .npmignore | 2 -- package.json | 3 ++- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/.npmignore b/.npmignore index 8d019b34a7..258b53f654 100644 --- a/.npmignore +++ b/.npmignore @@ -3,14 +3,12 @@ reports/ resolvers/ # config -.babelrc .eslintrc.yml .travis.yml appveyor.yml .coveralls.yml .editorconfig .gitignore -gulpfile.js # project stuff *.sublime-* diff --git a/package.json b/package.json index ae63924278..b5b1667e09 100644 --- a/package.json +++ b/package.json @@ -9,10 +9,11 @@ "scripts": { "watch": "NODE_PATH=./lib gulp watch-test", "cover": "gulp pretest && NODE_PATH=./lib istanbul cover --dir reports/coverage _mocha tests/lib/ -- --recursive -R progress", + "pretest": "eslint ./src", "test": "NODE_PATH=./lib gulp test", "ci-test": "eslint ./src && gulp pretest && istanbul cover --report lcovonly --dir reports/coverage _mocha tests/lib/ -- --recursive --reporter dot", "debug": "NODE_PATH=./lib mocha debug --recursive --reporter dot tests/lib/", - "prepublish": "eslint ./src && gulp prepublish", + "prepublish": "gulp prepublish", "coveralls": "cat ./reports/coverage/lcov.info | coveralls" }, "repository": { From bf0f5a2bf17228679e9cc90640fe9b8e9fd831b4 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Mon, 22 Feb 2016 08:30:03 -0500 Subject: [PATCH 11/14] =?UTF-8?q?lint=20as=20post-test=20action.=20don't?= =?UTF-8?q?=20care=20if=20linter=20is=20good=20if=20tests=20are=20failing.?= =?UTF-8?q?=20=F0=9F=98=85?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index b5b1667e09..2c5151fddb 100644 --- a/package.json +++ b/package.json @@ -9,7 +9,7 @@ "scripts": { "watch": "NODE_PATH=./lib gulp watch-test", "cover": "gulp pretest && NODE_PATH=./lib istanbul cover --dir reports/coverage _mocha tests/lib/ -- --recursive -R progress", - "pretest": "eslint ./src", + "posttest": "eslint ./src", "test": "NODE_PATH=./lib gulp test", "ci-test": "eslint ./src && gulp pretest && istanbul cover --report lcovonly --dir reports/coverage _mocha tests/lib/ -- --recursive --reporter dot", "debug": "NODE_PATH=./lib mocha debug --recursive --reporter dot tests/lib/", From 84bafeb64d5490e58dd379a3511ba788b66a640f Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Tue, 23 Feb 2016 07:03:05 -0500 Subject: [PATCH 12/14] flag deprecation at the identifier usages, too. good for long files --- src/core/declaredScope.js | 11 +++++ src/rules/namespace.js | 15 +------ src/rules/no-deprecated.js | 69 +++++++++++++++++++++++++++----- tests/src/rules/no-deprecated.js | 41 +++++++++++++++++-- 4 files changed, 110 insertions(+), 26 deletions(-) create mode 100644 src/core/declaredScope.js diff --git a/src/core/declaredScope.js b/src/core/declaredScope.js new file mode 100644 index 0000000000..11575f4cb7 --- /dev/null +++ b/src/core/declaredScope.js @@ -0,0 +1,11 @@ +export default function declaredScope(context, name) { + let references = context.getScope().references + , i + for (i = 0; i < references.length; i++) { + if (references[i].identifier.name === name) { + break + } + } + if (!references[i]) return undefined + return references[i].resolved.scope.type +} diff --git a/src/rules/namespace.js b/src/rules/namespace.js index 2d78c6a966..45ce8ada5d 100644 --- a/src/rules/namespace.js +++ b/src/rules/namespace.js @@ -1,5 +1,6 @@ import Exports from '../core/getExports' import importDeclaration from '../importDeclaration' +import declaredScope from '../core/declaredScope' module.exports = function (context) { @@ -30,18 +31,6 @@ module.exports = function (context) { namespace.name + '.' } - function declaredScope(name) { - let references = context.getScope().references - , i - for (i = 0; i < references.length; i++) { - if (references[i].identifier.name === name) { - break - } - } - if (!references[i]) return undefined - return references[i].resolved.scope.type - } - return { 'ImportNamespaceSpecifier': function (namespace) { const imports = getImportsAndReport(namespace) @@ -88,7 +77,7 @@ module.exports = function (context) { if (!namespaces.has(init.name)) return // check for redefinition in intermediate scopes - if (declaredScope(init.name) !== 'module') return + if (declaredScope(context, init.name) !== 'module') return const namespace = namespaces.get(init.name) diff --git a/src/rules/no-deprecated.js b/src/rules/no-deprecated.js index dd860f12db..28b4d84cba 100644 --- a/src/rules/no-deprecated.js +++ b/src/rules/no-deprecated.js @@ -1,6 +1,9 @@ import Exports from '../core/getExports' +import declaredScope from '../core/declaredScope' module.exports = function (context) { + const deprecated = new Map() + function checkSpecifiers(node) { if (node.source == null) return // local export, ignore @@ -12,29 +15,75 @@ module.exports = function (context) { return } + function getDeprecation(imported) { + const metadata = imports.named.get(imported) + if (!metadata || !metadata.doc) return + + let deprecation + if (metadata.doc.tags.some(t => t.title === 'deprecated' && (deprecation = t))) { + return deprecation + } + } + node.specifiers.forEach(function (im) { - let imported + let imported, local switch (im.type) { - case 'ImportDefaultSpecifier': imported = 'default'; break - case 'ImportSpecifier': imported = im.imported.name; break + + // case 'ImportNamespaceSpecifier':{ + // const submap = new Map() + // for (let name in imports.named) { + // const deprecation = getDeprecation(name) + // if (!deprecation) continue + // submap.set(name, deprecation) + // } + // if (submap.size > 0) deprecated.set(im.local.name, submap) + // return + // } + + case 'ImportDefaultSpecifier': + imported = 'default' + local = im.local.name + break + + case 'ImportSpecifier': + imported = im.imported.name + local = im.local.name + break + default: return // can't handle this one } // unknown thing can't be deprecated if (!imports.named.has(imported)) return - const metadata = imports.named.get(imported) - if (!metadata || !metadata.doc) return + const deprecation = getDeprecation(imported) + if (!deprecation) return + + context.report({ node: im, message: message(deprecation) }) + + deprecated.set(local, deprecation) - let deprecation - if (metadata.doc.tags.some(t => t.title === 'deprecated' && (deprecation = t))) { - context.report(im, - 'Deprecated' + (deprecation.description ? ': ' + deprecation.description : '.')) - } }) } return { 'ImportDeclaration': checkSpecifiers, + + 'Identifier': function (node) { + // ignore specifier identifiers + if (node.parent.type.slice(0, 6) === 'Import') return + + if (!deprecated.has(node.name)) return + + if (declaredScope(context, node.name) !== 'module') return + context.report({ + node, + message: message(deprecated.get(node.name)), + }) + }, } } + +function message(deprecation) { + return 'Deprecated' + (deprecation.description ? ': ' + deprecation.description : '.') +} diff --git a/tests/src/rules/no-deprecated.js b/tests/src/rules/no-deprecated.js index 0788192abc..a14e9f961c 100644 --- a/tests/src/rules/no-deprecated.js +++ b/tests/src/rules/no-deprecated.js @@ -1,4 +1,3 @@ -import * as path from 'path' import { test } from '../utils' import { RuleTester } from 'eslint' @@ -13,19 +12,55 @@ ruleTester.run('no-deprecated', rule, { test({ code: "import { fine } from './deprecated'" }), test({ code: "import { _undocumented } from './deprecated'" }), + + // naked namespace is fine + test({ code: "import * as depd from './deprecated'" }), ], invalid: [ + test({ code: "import { fn } from './deprecated'", errors: ["Deprecated: please use 'x' instead."], }), + test({ code: "import TerribleClass from './deprecated'", - errors: ["Deprecated: this is awful, use NotAsBadClass."], + errors: ['Deprecated: this is awful, use NotAsBadClass.'], }), + test({ code: "import { MY_TERRIBLE_ACTION } from './deprecated'", - errors: ["Deprecated: please stop sending/handling this action type."], + errors: ['Deprecated: please stop sending/handling this action type.'], + }), + + // ignore redeclares + test({ + code: "import { MY_TERRIBLE_ACTION } from './deprecated'; function shadow(MY_TERRIBLE_ACTION) { console.log(MY_TERRIBLE_ACTION); }", + errors: ['Deprecated: please stop sending/handling this action type.'], + }), + + // ignore non-deprecateds + test({ + code: "import { MY_TERRIBLE_ACTION, fine } from './deprecated'; console.log(fine)", + errors: ['Deprecated: please stop sending/handling this action type.'], + }), + + // reflag on subsequent usages + test({ + code: "import { MY_TERRIBLE_ACTION } from './deprecated'; console.log(MY_TERRIBLE_ACTION)", + errors: [ + { type: 'ImportSpecifier', message: 'Deprecated: please stop sending/handling this action type.' }, + { type: 'Identifier', message: 'Deprecated: please stop sending/handling this action type.' }, + ], + }), + + // works for function calls too + test({ + code: "import { MY_TERRIBLE_ACTION } from './deprecated'; console.log(MY_TERRIBLE_ACTION(this, is, the, worst))", + errors: [ + { type: 'ImportSpecifier', message: 'Deprecated: please stop sending/handling this action type.' }, + { type: 'Identifier', message: 'Deprecated: please stop sending/handling this action type.' }, + ], }), ], }) From d7418d64f5dc6b29be06d6720503929fcf3b2884 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Tue, 23 Feb 2016 07:07:37 -0500 Subject: [PATCH 13/14] updated no-deprecated worklist --- docs/rules/no-deprecated.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/rules/no-deprecated.md b/docs/rules/no-deprecated.md index 6702a7beac..7836bbfe5f 100644 --- a/docs/rules/no-deprecated.md +++ b/docs/rules/no-deprecated.md @@ -24,10 +24,17 @@ will report as such: ```js import { multiply } from './answer' // Deprecated: need to restart the experiment + +function whatever(y, z) { + return multiply(y, z) // Deprecated: need to restart the experiment +} ``` ### Worklist - [x] report explicit imports on the import node - [ ] support namespaces -- [ ] report explicit imports at reference time (at the identifier) similar to namespace + - [ ] should bubble up through deep namespaces (#157) +- [x] report explicit imports at reference time (at the identifier) similar to namespace +- [ ] mark module deprecated if file JSDoc has a @deprecated tag? + From df32e652fb32010a5031d1ffe5021a1884d24f34 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Tue, 23 Feb 2016 07:34:18 -0500 Subject: [PATCH 14/14] support full deprecated module --- docs/rules/no-deprecated.md | 2 +- src/core/getExports.js | 14 ++++ src/rules/no-deprecated.js | 6 ++ tests/files/deprecated-file.js | 12 ++++ tests/src/core/getExports.js | 117 ++++++++++++++++++------------- tests/src/rules/no-deprecated.js | 8 +++ 6 files changed, 108 insertions(+), 51 deletions(-) create mode 100644 tests/files/deprecated-file.js diff --git a/docs/rules/no-deprecated.md b/docs/rules/no-deprecated.md index 7836bbfe5f..d5be8fa465 100644 --- a/docs/rules/no-deprecated.md +++ b/docs/rules/no-deprecated.md @@ -36,5 +36,5 @@ function whatever(y, z) { - [ ] support namespaces - [ ] should bubble up through deep namespaces (#157) - [x] report explicit imports at reference time (at the identifier) similar to namespace -- [ ] mark module deprecated if file JSDoc has a @deprecated tag? +- [x] mark module deprecated if file JSDoc has a @deprecated tag? diff --git a/src/core/getExports.js b/src/core/getExports.js index 38e7cdeeb9..704f38e244 100644 --- a/src/core/getExports.js +++ b/src/core/getExports.js @@ -81,6 +81,20 @@ export default class ExportMap { return m // can't continue } + // attempt to collect module doc + ast.comments.some(c => { + if (c.type !== 'Block') return false + try { + const doc = doctrine.parse(c.value, { unwrap: true }) + if (doc.tags.some(t => t.title === 'module')) { + m.doc = doc + return true + } + } catch (err) { /* ignore */ } + return false + }) + + ast.body.forEach(function (n) { if (n.type === 'ExportDefaultDeclaration') { diff --git a/src/rules/no-deprecated.js b/src/rules/no-deprecated.js index 28b4d84cba..b6168fd747 100644 --- a/src/rules/no-deprecated.js +++ b/src/rules/no-deprecated.js @@ -10,6 +10,12 @@ module.exports = function (context) { const imports = Exports.get(node.source.value, context) if (imports == null) return + let moduleDeprecation + if (imports.doc && + imports.doc.tags.some(t => t.title === 'deprecated' && (moduleDeprecation = t))) { + context.report({ node, message: message(moduleDeprecation) }) + } + if (imports.errors.length) { imports.reportErrors(context, node) return diff --git a/tests/files/deprecated-file.js b/tests/files/deprecated-file.js new file mode 100644 index 0000000000..b27a915ac1 --- /dev/null +++ b/tests/files/deprecated-file.js @@ -0,0 +1,12 @@ +/** + * ugh! + * @module some/deprecated + * @deprecated this module is the worst. + */ + +/** + * this class is great + */ +export default class Thing { } + +// some other comment diff --git a/tests/src/core/getExports.js b/tests/src/core/getExports.js index df82d4c8ea..9727f39a39 100644 --- a/tests/src/core/getExports.js +++ b/tests/src/core/getExports.js @@ -90,69 +90,86 @@ describe('getExports', function () { context('deprecation metadata', function () { function jsdocTests(parseContext) { - let imports - before('parse file', function () { - imports = ExportMap.parse( - getFilename('deprecated.js'), parseContext) - - // sanity checks - expect(imports.errors).to.be.empty - }) - - it('works with named imports.', function () { - expect(imports.named.has('fn')).to.be.true - - expect(imports.named.get('fn')) - .to.have.deep.property('doc.tags[0].title', 'deprecated') - expect(imports.named.get('fn')) - .to.have.deep.property('doc.tags[0].description', "please use 'x' instead.") - }) + context('deprecated imports', function () { + let imports + before('parse file', function () { + imports = ExportMap.parse( + getFilename('deprecated.js'), parseContext) + + // sanity checks + expect(imports.errors).to.be.empty + }) - it('works with default imports.', function () { - expect(imports.named.has('default')).to.be.true - const importMeta = imports.named.get('default') + it('works with named imports.', function () { + expect(imports.named.has('fn')).to.be.true - expect(importMeta).to.have.deep.property('doc.tags[0].title', 'deprecated') - expect(importMeta).to.have.deep.property('doc.tags[0].description', 'this is awful, use NotAsBadClass.') - }) + expect(imports.named.get('fn')) + .to.have.deep.property('doc.tags[0].title', 'deprecated') + expect(imports.named.get('fn')) + .to.have.deep.property('doc.tags[0].description', "please use 'x' instead.") + }) - it('works with variables.', function () { - expect(imports.named.has('MY_TERRIBLE_ACTION')).to.be.true - const importMeta = imports.named.get('MY_TERRIBLE_ACTION') + it('works with default imports.', function () { + expect(imports.named.has('default')).to.be.true + const importMeta = imports.named.get('default') - expect(importMeta).to.have.deep.property( - 'doc.tags[0].title', 'deprecated') - expect(importMeta).to.have.deep.property( - 'doc.tags[0].description', 'please stop sending/handling this action type.') - }) + expect(importMeta).to.have.deep.property('doc.tags[0].title', 'deprecated') + expect(importMeta).to.have.deep.property('doc.tags[0].description', 'this is awful, use NotAsBadClass.') + }) - context('multi-line variables', function () { - it('works for the first one', function () { - expect(imports.named.has('CHAIN_A')).to.be.true - const importMeta = imports.named.get('CHAIN_A') + it('works with variables.', function () { + expect(imports.named.has('MY_TERRIBLE_ACTION')).to.be.true + const importMeta = imports.named.get('MY_TERRIBLE_ACTION') expect(importMeta).to.have.deep.property( 'doc.tags[0].title', 'deprecated') expect(importMeta).to.have.deep.property( - 'doc.tags[0].description', 'this chain is awful') + 'doc.tags[0].description', 'please stop sending/handling this action type.') }) - it('works for the second one', function () { - expect(imports.named.has('CHAIN_B')).to.be.true - const importMeta = imports.named.get('CHAIN_B') - expect(importMeta).to.have.deep.property( - 'doc.tags[0].title', 'deprecated') - expect(importMeta).to.have.deep.property( - 'doc.tags[0].description', 'so awful') + context('multi-line variables', function () { + it('works for the first one', function () { + expect(imports.named.has('CHAIN_A')).to.be.true + const importMeta = imports.named.get('CHAIN_A') + + expect(importMeta).to.have.deep.property( + 'doc.tags[0].title', 'deprecated') + expect(importMeta).to.have.deep.property( + 'doc.tags[0].description', 'this chain is awful') + }) + it('works for the second one', function () { + expect(imports.named.has('CHAIN_B')).to.be.true + const importMeta = imports.named.get('CHAIN_B') + + expect(importMeta).to.have.deep.property( + 'doc.tags[0].title', 'deprecated') + expect(importMeta).to.have.deep.property( + 'doc.tags[0].description', 'so awful') + }) + it('works for the third one, etc.', function () { + expect(imports.named.has('CHAIN_C')).to.be.true + const importMeta = imports.named.get('CHAIN_C') + + expect(importMeta).to.have.deep.property( + 'doc.tags[0].title', 'deprecated') + expect(importMeta).to.have.deep.property( + 'doc.tags[0].description', 'still terrible') + }) }) - it('works for the third one, etc.', function () { - expect(imports.named.has('CHAIN_C')).to.be.true - const importMeta = imports.named.get('CHAIN_C') + }) - expect(importMeta).to.have.deep.property( - 'doc.tags[0].title', 'deprecated') - expect(importMeta).to.have.deep.property( - 'doc.tags[0].description', 'still terrible') + context('full module', function () { + let imports + before('parse file', function () { + imports = ExportMap.parse( + getFilename('deprecated-file.js'), parseContext) + + // sanity checks + expect(imports.errors).to.be.empty + }) + + it('has JSDoc metadata', function () { + expect(imports.doc).to.exist }) }) } diff --git a/tests/src/rules/no-deprecated.js b/tests/src/rules/no-deprecated.js index a14e9f961c..8c225ad931 100644 --- a/tests/src/rules/no-deprecated.js +++ b/tests/src/rules/no-deprecated.js @@ -62,5 +62,13 @@ ruleTester.run('no-deprecated', rule, { { type: 'Identifier', message: 'Deprecated: please stop sending/handling this action type.' }, ], }), + + // deprecated full module + test({ + code: "import Thing from './deprecated-file'", + errors: [ + { type: 'ImportDeclaration', message: 'Deprecated: this module is the worst.' }, + ], + }), ], })