From 3c506917d38f498f780de2d2b4acdca189fd1e76 Mon Sep 17 00:00:00 2001 From: William Durand Date: Mon, 8 Jun 2020 16:07:13 +0200 Subject: [PATCH] Reconfigure webpack to export javascript rule files separately --- .npmignore | 3 +- .travis.yml | 2 +- README.md | 4 +- package-lock.json | 32 ++++++- src/main.js | 6 +- .../javascript/content-scripts-file-absent.js | 2 +- src/rules/javascript/deprecated-entities.js | 4 +- src/rules/javascript/event-listener-fourth.js | 2 +- src/rules/javascript/global-require-arg.js | 5 +- src/rules/javascript/opendialog-nonlit-uri.js | 2 +- src/rules/javascript/opendialog-remote-uri.js | 2 +- .../webextension-api-compat-android.js | 2 +- .../javascript/webextension-api-compat.js | 2 +- src/rules/javascript/webextension-api.js | 2 +- .../javascript/webextension-deprecated-api.js | 2 +- .../webextension-unsupported-api.js | 2 +- src/scanners/javascript.js | 20 +++-- .../javascript/message-rule/message-rule.js | 9 ++ .../metadata-not-passed.js | 17 ++++ tests/setup.js | 9 ++ tests/unit/helpers.js | 19 +++-- tests/unit/scanners/test.javascript.js | 84 +++---------------- webpack.config.js | 18 +++- 23 files changed, 141 insertions(+), 109 deletions(-) create mode 100644 tests/fixtures/rules/javascript/message-rule/message-rule.js create mode 100644 tests/fixtures/rules/javascript/metadata-not-passed/metadata-not-passed.js diff --git a/.npmignore b/.npmignore index b0b1e3dd6c3..b46d48e6328 100644 --- a/.npmignore +++ b/.npmignore @@ -7,4 +7,5 @@ !README.md !bin/addons-linter !dist/addons-linter.* -!dist/locale/** \ No newline at end of file +!dist/locale/** +!dist/rules/** diff --git a/.travis.yml b/.travis.yml index c81ff387272..01c9de310d3 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,9 +3,9 @@ node_js: - 'v10.21.0' - 'v12.18.0' script: - - npm run test-ci - node scripts/build-locales - npm run build + - npm run test-ci - npm run lint # run integration tests using an addons-linter binary in a production-like environment - npm run test-integration:production diff --git a/README.md b/README.md index 2177f6fe2d7..e3e2d3bb78a 100644 --- a/README.md +++ b/README.md @@ -109,6 +109,8 @@ If you have Node.js installed, here's the quick start to getting your developmen git clone https://github.com/mozilla/addons-linter.git cd addons-linter npm install +# Build the project. +npm run build # Run the test-suite and watch for changes. Use `npm run test-once` to # just run it once. npm run test @@ -156,7 +158,7 @@ Dependencies are automatically kept up-to-date using [renovatebot](https://renov ### Building -You can run `npm build` to build the library. +You can run `npm run build` to build the library. Once you build the library you can use the CLI in `bin/addons-linter`. diff --git a/package-lock.json b/package-lock.json index 45d2f420fa4..ebc4b84ca3a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "addons-linter", - "version": "1.23.0", + "version": "1.25.0", "lockfileVersion": 1, "requires": true, "dependencies": { @@ -2671,6 +2671,16 @@ "dev": true, "optional": true }, + "bindings": { + "version": "1.5.0", + "resolved": "https://registry.npmjs.org/bindings/-/bindings-1.5.0.tgz", + "integrity": "sha512-p2q/t/mhvuOj/UeLlV6566GD/guowlr0hHxClI0W9m7MWYkL1F0hLo+0Aexs9HSPCtR1SXQ0TD3MMKrXZajbiQ==", + "dev": true, + "optional": true, + "requires": { + "file-uri-to-path": "1.0.0" + } + }, "bl": { "version": "4.0.2", "resolved": "https://registry.npmjs.org/bl/-/bl-4.0.2.tgz", @@ -3044,7 +3054,11 @@ "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-1.2.13.tgz", "integrity": "sha512-oWb1Z6mkHIskLzEJ/XWX0srkpkTQ7vaopMQkyaEIoq0fmtFVxOthb8cCxeT+p3ynTdkk/RZwbgG4brR5BeWECw==", "dev": true, - "optional": true + "optional": true, + "requires": { + "bindings": "^1.5.0", + "nan": "^2.12.1" + } } } }, @@ -4992,6 +5006,13 @@ "flat-cache": "^2.0.1" } }, + "file-uri-to-path": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/file-uri-to-path/-/file-uri-to-path-1.0.0.tgz", + "integrity": "sha512-0Zt+s3L7Vf1biwWZ29aARiVYLx7iMGnEUl9x33fbB/j3jR81u/O2LbqK+Bm1CDSNDKVtJ/YjwY7TUd5SkeLQLw==", + "dev": true, + "optional": true + }, "filename-reserved-regex": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/filename-reserved-regex/-/filename-reserved-regex-1.0.0.tgz", @@ -7750,6 +7771,13 @@ "resolved": "https://registry.npmjs.org/mute-stream/-/mute-stream-0.0.8.tgz", "integrity": "sha512-nnbWWOkoWyUsTjKrhgD0dcz22mdkSnpYqbEjIm2nhwhuxlSkpywJmBo8h0ZqJdkp73mb90SssHkN4rsRaBAfAA==" }, + "nan": { + "version": "2.14.1", + "resolved": "https://registry.npmjs.org/nan/-/nan-2.14.1.tgz", + "integrity": "sha512-isWHgVjnFjh2x2yuJ/tj3JbwoHu3UC2dX5G/88Cm24yB6YopVgxvBObDY7n5xW6ExmFhJpSEQqFPvq9zaXc8Jw==", + "dev": true, + "optional": true + }, "nanomatch": { "version": "1.2.13", "resolved": "https://registry.npmjs.org/nanomatch/-/nanomatch-1.2.13.tgz", diff --git a/src/main.js b/src/main.js index 143e9debfef..0eed218e6b7 100644 --- a/src/main.js +++ b/src/main.js @@ -17,4 +17,8 @@ export function createInstance({ return new Linter(config); } -export default Linter; +export default { + Linter, + createInstance, + isRunFromCLI, +}; diff --git a/src/rules/javascript/content-scripts-file-absent.js b/src/rules/javascript/content-scripts-file-absent.js index bfa0bab29f0..6828ab015a1 100644 --- a/src/rules/javascript/content-scripts-file-absent.js +++ b/src/rules/javascript/content-scripts-file-absent.js @@ -6,7 +6,7 @@ import { CONTENT_SCRIPT_EMPTY, } from 'messages/javascript'; -module.exports = { +export default { create(context) { const existingFiles = Object.keys(context.settings.existingFiles || {}).map( (fileName) => { diff --git a/src/rules/javascript/deprecated-entities.js b/src/rules/javascript/deprecated-entities.js index 81909c57a02..24692bdb9ab 100644 --- a/src/rules/javascript/deprecated-entities.js +++ b/src/rules/javascript/deprecated-entities.js @@ -9,9 +9,7 @@ export const DEPRECATED_ENTITIES = [ }, ]; -module.exports = { - DEPRECATED_ENTITIES, - +export default { create(context) { return { // eslint-disable-next-line consistent-return diff --git a/src/rules/javascript/event-listener-fourth.js b/src/rules/javascript/event-listener-fourth.js index 8ec69d926a9..5bdb1711084 100644 --- a/src/rules/javascript/event-listener-fourth.js +++ b/src/rules/javascript/event-listener-fourth.js @@ -1,7 +1,7 @@ import { EVENT_LISTENER_FOURTH } from 'messages/javascript'; import { getNodeReference } from 'utils'; -module.exports = { +export default { create(context) { return { // eslint-disable-next-line consistent-return diff --git a/src/rules/javascript/global-require-arg.js b/src/rules/javascript/global-require-arg.js index 53bd77f7c45..b0b57ec0811 100644 --- a/src/rules/javascript/global-require-arg.js +++ b/src/rules/javascript/global-require-arg.js @@ -2,10 +2,9 @@ import { UNEXPECTED_GLOGAL_ARG } from 'messages'; import { getVariable } from 'utils'; /* - * This rule will detect a global passed to `require()` as the first arg - * + * This rule will detect a global passed to `require()` as the first argument. */ -module.exports = { +export default { create(context) { return { // eslint-disable-next-line consistent-return diff --git a/src/rules/javascript/opendialog-nonlit-uri.js b/src/rules/javascript/opendialog-nonlit-uri.js index 413db17c186..093cb0ea35e 100644 --- a/src/rules/javascript/opendialog-nonlit-uri.js +++ b/src/rules/javascript/opendialog-nonlit-uri.js @@ -1,6 +1,6 @@ import { OPENDIALOG_NONLIT_URI } from 'messages'; -module.exports = { +export default { create(context) { return { // eslint-disable-next-line consistent-return diff --git a/src/rules/javascript/opendialog-remote-uri.js b/src/rules/javascript/opendialog-remote-uri.js index 2b85cfde1d7..19f12110d42 100644 --- a/src/rules/javascript/opendialog-remote-uri.js +++ b/src/rules/javascript/opendialog-remote-uri.js @@ -1,7 +1,7 @@ import { isLocalUrl } from 'utils'; import { OPENDIALOG_REMOTE_URI } from 'messages'; -module.exports = { +export default { create(context) { return { // eslint-disable-next-line consistent-return diff --git a/src/rules/javascript/webextension-api-compat-android.js b/src/rules/javascript/webextension-api-compat-android.js index f658e414740..6f2ea06f553 100644 --- a/src/rules/javascript/webextension-api-compat-android.js +++ b/src/rules/javascript/webextension-api-compat-android.js @@ -4,7 +4,7 @@ import { ANDROID_INCOMPATIBLE_API } from 'messages/javascript'; import { createCompatibilityRule } from 'utils'; import { hasBrowserApi } from 'schema/browser-apis'; -module.exports = { +export default { create(context) { return createCompatibilityRule( 'firefox_android', diff --git a/src/rules/javascript/webextension-api-compat.js b/src/rules/javascript/webextension-api-compat.js index cc91059b980..a67f48a93e0 100644 --- a/src/rules/javascript/webextension-api-compat.js +++ b/src/rules/javascript/webextension-api-compat.js @@ -4,7 +4,7 @@ import { INCOMPATIBLE_API } from 'messages/javascript'; import { createCompatibilityRule } from 'utils'; import { hasBrowserApi } from 'schema/browser-apis'; -module.exports = { +export default { create(context) { return createCompatibilityRule( 'firefox', diff --git a/src/rules/javascript/webextension-api.js b/src/rules/javascript/webextension-api.js index b46a4bdb960..07c9ea1060b 100644 --- a/src/rules/javascript/webextension-api.js +++ b/src/rules/javascript/webextension-api.js @@ -1,7 +1,7 @@ import { isTemporaryApi } from 'schema/browser-apis'; import { apiToMessage, isBrowserNamespace } from 'utils'; -module.exports = { +export default { create(context) { return { // eslint-disable-next-line consistent-return diff --git a/src/rules/javascript/webextension-deprecated-api.js b/src/rules/javascript/webextension-deprecated-api.js index 0a872a56109..a9cf07dc51c 100644 --- a/src/rules/javascript/webextension-deprecated-api.js +++ b/src/rules/javascript/webextension-deprecated-api.js @@ -3,7 +3,7 @@ import { DEPRECATED_JAVASCRIPT_APIS } from 'const'; import { isDeprecatedApi, hasBrowserApi } from 'schema/browser-apis'; import { isBrowserNamespace } from 'utils'; -module.exports = { +export default { create(context) { return { MemberExpression(node) { diff --git a/src/rules/javascript/webextension-unsupported-api.js b/src/rules/javascript/webextension-unsupported-api.js index d497959421b..d739b6598bb 100644 --- a/src/rules/javascript/webextension-unsupported-api.js +++ b/src/rules/javascript/webextension-unsupported-api.js @@ -2,7 +2,7 @@ import { UNSUPPORTED_API } from 'messages/javascript'; import { hasBrowserApi } from 'schema/browser-apis'; import { isBrowserNamespace } from 'utils'; -module.exports = { +export default { create(context) { return { MemberExpression(node) { diff --git a/src/scanners/javascript.js b/src/scanners/javascript.js index bcd2c496bb8..043cee347bf 100644 --- a/src/scanners/javascript.js +++ b/src/scanners/javascript.js @@ -1,3 +1,4 @@ +/* global appRoot */ import path from 'path'; import ESLint from 'eslint'; @@ -39,10 +40,15 @@ export default class JavaScriptScanner { return 'javascript'; } - async scan( + async scan({ _ESLint = ESLint, - { _ruleMapping = ESLINT_RULE_MAPPING, _messages = messages } = {} - ) { + _messages = messages, + _ruleMapping = ESLINT_RULE_MAPPING, + // This tells ESLint where to expect the ESLint rules for addons-linter. + // Its default value is defined below. This property is mainly used for + // testing purposes. + _rulePaths = undefined, + } = {}) { this.sourceType = this.detectSourceType(this.filename); const rules = {}; @@ -64,7 +70,11 @@ export default class JavaScriptScanner { sourceType: this.sourceType, }, rules, - rulePaths: [path.join(__dirname, '..', 'rules', 'javascript')], + // The default value for `rulePaths` is configured so that it finds the + // files exported by webpack when this project is built. + rulePaths: _rulePaths || [ + path.join(appRoot, 'dist', 'rules', 'javascript'), + ], plugins: ['no-unsanitized'], allowInlineConfig: false, @@ -104,7 +114,7 @@ export default class JavaScriptScanner { if (typeof message.message === 'undefined') { throw new Error( oneLine`JS rules must pass a valid message as - the second argument to context.report()` + the second argument to context.report()` ); } diff --git a/tests/fixtures/rules/javascript/message-rule/message-rule.js b/tests/fixtures/rules/javascript/message-rule/message-rule.js new file mode 100644 index 00000000000..3723af0f8a8 --- /dev/null +++ b/tests/fixtures/rules/javascript/message-rule/message-rule.js @@ -0,0 +1,9 @@ +module.exports = { + create: (context) => { + return { + MemberExpression(node) { + context.report(node, 'this is the message'); + }, + }; + }, +}; diff --git a/tests/fixtures/rules/javascript/metadata-not-passed/metadata-not-passed.js b/tests/fixtures/rules/javascript/metadata-not-passed/metadata-not-passed.js new file mode 100644 index 00000000000..c9ce70d2f5d --- /dev/null +++ b/tests/fixtures/rules/javascript/metadata-not-passed/metadata-not-passed.js @@ -0,0 +1,17 @@ +module.exports = { + create: (context) => { + return { + Identifier: (node) => { + const metadata = context.settings.addonMetadata; + + if (typeof metadata !== 'object') { + context.report(node, 'Metadata should be an object.'); + } + + if (metadata.guid !== 'snowflake') { + context.report(node, 'Metadata properties not present.'); + } + } + } + } +}; diff --git a/tests/setup.js b/tests/setup.js index 17bc699fb15..d89358b94f3 100644 --- a/tests/setup.js +++ b/tests/setup.js @@ -1,3 +1,6 @@ +import fs from 'fs'; +import path from 'path'; + import sinon from 'sinon'; // Setup sinon global to be a sandbox which is restored @@ -22,6 +25,12 @@ jest.mock('cli', () => { }; }); +global.appRoot = path.join(__dirname, '..'); + +if (!fs.existsSync(path.join(global.appRoot, 'dist'))) { + throw new Error('Please run `npm run build` before running the test suite.'); +} + afterEach(() => { global.sinon.restore(); }); diff --git a/tests/unit/helpers.js b/tests/unit/helpers.js index 616638ada39..ea001d91a43 100644 --- a/tests/unit/helpers.js +++ b/tests/unit/helpers.js @@ -1,3 +1,4 @@ +import path from 'path'; import fs from 'fs'; import { Readable, Stream } from 'stream'; import { assert } from 'assert'; @@ -240,17 +241,17 @@ export function getStreamableIO(files) { getFiles: () => { return Promise.resolve(files); }, - getFileAsStream: (path) => { + getFileAsStream: (_path) => { if ( - files[path] instanceof Stream && - typeof files[path]._read === 'function' && - typeof files[path]._readableState === 'object' + files[_path] instanceof Stream && + typeof files[_path]._read === 'function' && + typeof files[_path]._readableState === 'object' ) { - return Promise.resolve(files[path]); + return Promise.resolve(files[_path]); } const stream = new Readable(); - stream.push(files[path]); + stream.push(files[_path]); stream.push(null); return Promise.resolve(stream); }, @@ -381,3 +382,9 @@ export function replacePlaceholders(text, data) { } ); } + +export const FIXTURES_DIR = path.join(__dirname, '..', 'fixtures'); + +export const getJsRulePathForRule = (ruleName) => { + return path.join(FIXTURES_DIR, 'rules', 'javascript', ruleName); +}; diff --git a/tests/unit/scanners/test.javascript.js b/tests/unit/scanners/test.javascript.js index aab860ef9ac..9a6d9cda4c6 100644 --- a/tests/unit/scanners/test.javascript.js +++ b/tests/unit/scanners/test.javascript.js @@ -15,6 +15,7 @@ import JavaScriptScanner from 'scanners/javascript'; import Linter from 'linter'; import { + getJsRulePathForRule, fakeMessageData, getRuleFiles, getVariable, @@ -28,18 +29,6 @@ const linterMock = { }), }; -const esLintMock = { - CLIEngine: (engineOptions) => { - return { - engineOptions, - linter: linterMock, - executeOnText: () => ({ - results: [], - }), - }; - }, -}; - describe('JavaScript Scanner', () => { it('should report a proper scanner name', () => { expect(JavaScriptScanner.scannerName).toEqual('javascript'); @@ -193,7 +182,7 @@ describe('JavaScript Scanner', () => { const jsScanner = new JavaScriptScanner('whatever', 'badcode.js'); - await expect(jsScanner.scan(FakeESLint)).rejects.toThrow( + await expect(jsScanner.scan({ _ESLint: FakeESLint })).rejects.toThrow( /JS rules must pass a valid message/ ); }); @@ -237,9 +226,7 @@ describe('JavaScript Scanner', () => { }); // eslint-disable-next-line jest/no-disabled-tests - it.skip('should pass addon metadata to rules', async () => { - const fakeRules = { 'metadata-not-passed': { create: () => {} } }; - + it('should pass addon metadata to rules', async () => { const fakeMessages = { METADATA_NOT_PASSED: { ...fakeMessageData, @@ -253,37 +240,19 @@ describe('JavaScript Scanner', () => { }; const fakeESLintMapping = { 'metadata-not-passed': ESLINT_ERROR }; - sinon - .stub(fakeRules['metadata-not-passed'], 'create') - .callsFake((context) => { - return { - Identifier: () => { - const metadata = context.settings.addonMetadata; - - if (typeof metadata !== 'object') { - assert.fail(null, null, 'Metadata should be an object.'); - } - - if (metadata.guid !== 'snowflake') { - assert.fail(null, null, 'Metadata properties not present.'); - } - }, - }; - }); - const jsScanner = new JavaScriptScanner( 'var hello = "something";', 'index.html', fakeMetadata ); - await jsScanner.scan(ESLint, { - _rules: fakeRules, - _ruleMapping: fakeESLintMapping, + const { linterMessages } = await jsScanner.scan({ _messages: fakeMessages, + _ruleMapping: fakeESLintMapping, + _rulePaths: [getJsRulePathForRule('metadata-not-passed')], }); - sinon.assert.calledOnce(fakeRules['metadata-not-passed'].create); + expect(linterMessages).toEqual([]); }); it('should export all rules in rules/javascript', async () => { @@ -332,53 +301,20 @@ describe('JavaScript Scanner', () => { }); // eslint-disable-next-line jest/no-disabled-tests - it.skip('treats a non-code string message as the message', async () => { - const _rules = { - 'message-rule': (context) => { - return { - MemberExpression(node) { - context.report(node, 'this is the message'); - }, - }; - }, - }; + it('treats a non-code string message as the message', async () => { const _ruleMapping = { 'message-rule': ESLINT_ERROR }; const fakeMetadata = { addonMetadata: validMetadata({}) }; const jsScanner = new JavaScriptScanner('foo.bar', 'code.js', fakeMetadata); - const { linterMessages } = await jsScanner.scan(undefined, { - _rules, + const { linterMessages } = await jsScanner.scan({ _ruleMapping, + _rulePaths: [getJsRulePathForRule('message-rule')], }); expect(linterMessages.length).toEqual(1); expect(linterMessages[0].code).toEqual('this is the message'); expect(linterMessages[0].message).toEqual('this is the message'); }); - // eslint-disable-next-line jest/no-disabled-tests - describe.skip('scanner options tests', () => { - it('should define valid set of rules for linter', async () => { - const jsScanner = new JavaScriptScanner('', 'filename.txt', { - disabledRules: - 'no-eval, no-implied-eval, no-unsanitized/method', - }); - const original = linterMock.defineRule; - sinon.stub(linterMock, 'defineRule').callsFake(original); - await jsScanner.scan(esLintMock, { - _rules: { - test: {}, - 'no-eval': {}, - }, - _ruleMapping: { - test: {}, - 'no-eval': {}, - }, - }); - const spyCalls = linterMock.defineRule.getCalls(); - expect(spyCalls.length).toBe(1); - }); - }); - describe('detectSourceType', () => { it('should detect module', async () => { const code = oneLine` diff --git a/webpack.config.js b/webpack.config.js index 68132271a44..1e39e3ebe1d 100644 --- a/webpack.config.js +++ b/webpack.config.js @@ -1,5 +1,7 @@ const path = require('path'); +const glob = require('glob'); + // eslint-disable-next-line import/no-extraneous-dependencies const webpack = require('webpack'); // eslint-disable-next-line import/no-extraneous-dependencies @@ -9,12 +11,19 @@ module.exports = { // Set the webpack4 mode 'none' for compatibility with the behavior // of the webpack3 bundling step. mode: 'none', - entry: './src/main.js', + entry: { + 'addons-linter': './src/main.js', + ...glob.sync('./src/rules/javascript/*.js').reduce((acc, file) => { + acc[file.replace(/^\.\/src\//, '').replace('.js', '')] = file; + return acc; + }, {}), + }, target: 'node', output: { - path: path.join(__dirname, 'dist'), - filename: 'addons-linter.js', + filename: '[name].js', libraryTarget: 'commonjs2', + libraryExport: 'default', + path: path.join(__dirname, 'dist'), }, module: { rules: [ @@ -48,4 +57,7 @@ module.exports = { modules: ['src', 'node_modules'], }, devtool: 'sourcemap', + node: { + __dirname: false, + }, };