From 6a5007883dc8e02259fa89c700a3cd04c4ef67dc Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Wed, 21 Dec 2022 12:06:54 -0600 Subject: [PATCH 01/22] Implement no-implicit-injections lint rule chore: generate docs wip: setup for inital create Implement basic identification for implicit injections fix: failure to recognize module type or all modules feat: add fixing for no-implicit-injections update readme for no-implicit-injections --- README.md | 1 + docs/rules/no-implicit-injections.md | 121 ++++++++++++ lib/recommended-rules.js | 1 + lib/rules/no-implicit-injections.js | 176 +++++++++++++++++ package.json | 1 + tests/__snapshots__/recommended.js.snap | 1 + tests/lib/rules/no-implicit-injections.js | 228 ++++++++++++++++++++++ 7 files changed, 529 insertions(+) create mode 100644 docs/rules/no-implicit-injections.md create mode 100644 lib/rules/no-implicit-injections.js create mode 100644 tests/lib/rules/no-implicit-injections.js diff --git a/README.md b/README.md index 03844ce13a..d65e1ca023 100644 --- a/README.md +++ b/README.md @@ -102,6 +102,7 @@ module.exports = { | [new-module-imports](docs/rules/new-module-imports.md) | enforce using "New Module Imports" from Ember RFC #176 | ✅ | | | | [no-array-prototype-extensions](docs/rules/no-array-prototype-extensions.md) | disallow usage of Ember's `Array` prototype extensions | | 🔧 | | | [no-function-prototype-extensions](docs/rules/no-function-prototype-extensions.md) | disallow usage of Ember's `function` prototype extensions | ✅ | | | +| [no-implicit-injections](docs/rules/no-implicit-injections.md) | enforce usage of implicit service injections | ✅ | 🔧 | | | [no-mixins](docs/rules/no-mixins.md) | disallow the usage of mixins | ✅ | | | | [no-new-mixins](docs/rules/no-new-mixins.md) | disallow the creation of new mixins | ✅ | | | | [no-observers](docs/rules/no-observers.md) | disallow usage of observers | ✅ | | | diff --git a/docs/rules/no-implicit-injections.md b/docs/rules/no-implicit-injections.md new file mode 100644 index 0000000000..dd8409d050 --- /dev/null +++ b/docs/rules/no-implicit-injections.md @@ -0,0 +1,121 @@ +# ember/no-implicit-injections + +💼 This rule is enabled in the ✅ `recommended` [config](https://github.com/ember-cli/eslint-plugin-ember#-configurations). + +🔧 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix). + + + +✅ The `"extends": "plugin:ember/recommended"` property in a configuration file enables this rule. + +🔧 The `--fix` option on the [command line](https://eslint.org/docs/user-guide/command-line-interface#fixing-problems) can automatically fix some of the problems reported by this rule. + +Ember 3.26 introduced a deprecation for relying on implicit service injections or allowing addons to implicitly inject services into all classes of certain types. Support for this is dropped in Ember 4.0. + +In many applications, `this.store` from Ember Data is often used without injecting the `store` service in Controllers or Routes. Other addons may also have included implicit service injections via initializers and the `application.inject` API. + +To resolve this deprecation, a service should be explicitly declared and injected using the [service injection decorator](https://api.emberjs.com/ember/3.28/functions/@ember%2Fservice/inject). + +## Rule Details + +This rule checks for a configured list of previously auto injected services and warns if they are used in classes without explicit injected service properties. + +## Examples + +Examples of **incorrect** code for this rule: + +```js +// routes/index.js +import Route from '@ember/routing/route'; + +export default class IndexRoute extends Route { + async model() { + return this.store.findAll('rental'); + } +} + +``` + +```js +// controller/index.js +import Route from '@ember/routing/route'; +import { action } from '@ember/object'; + +export default class IndexController extends Controller { + @action + async loadUsers() { + return this.store.findAll('user'); + } +} +``` + +Examples of **correct** code for this rule: + +```js +// routes/index.js +import Route from '@ember/routing/route'; +import { inject as service } from '@ember/service'; + +export default class IndexRoute extends Route { + @service('store') store; + + async model() { + return this.store.findAll('rental'); + } +} +``` + +```js +// controller/index.js +import Route from '@ember/routing/route'; +import { action } from '@ember/object'; +import { inject as service } from '@ember/service'; + +export default class IndexController extends Controller { + @service('store') store; + + @action + async loadUsers() { + return this.store.findAll('user'); + } +} +``` + +## Migration + +The autofixer for this rule will update classes and add injections for the configured services. + +## Configuration + +This lint rule will search for instances of `store` used in routes or controllers by default. If you have other services that you would like to check for uses of, the configuration can be overridden. + +- object -- containing the following properties: + - array -- `services` -- Array of configuration objects configuring the lint rule to check for use of implicit injected services + - string -- `serviceName` -- The name of the service that is implicitly injected + - array -- `moduleNames` -- Array of string listing the types of classes (controller, route, component, etc) to check for implicit injections. If an array is declared only those class types will be checked for implicit injection. (Defaults to checking all class files/types) + +Example config: + +```js +module.exports = { + rules: { + 'ember/no-implicit-injections': { + services: [ + // Ember Responsive Used to Auto Inject the media service in Components/Controllers + { serviceName: 'media', moduleNames: ['Component', 'Controller'] }, + // Ember CLI Flash Used to Auto Inject the flashMessages service in all modules + { serviceName: 'flashMessages' }, + ] + } + } +} +``` + +## Related Rules + +- [no-unnecessary-service-injection-argument](https://github.com/ember-cli/eslint-plugin-ember/blob/master/docs/rules/no-unnecessary-service-injection-argument.md) omit service injection argument if possible +- [no-implicit-service-injection-argument](https://github.com/ember-cli/eslint-plugin-ember/blob/master/docs/rules/no-unnecessary-service-injection-argument.md) require the service injection argument for all services (fix output for this rule follows this pattern) + +## References + +- [Deprecation](https://deprecations.emberjs.com/v3.x/#toc_implicit-injections) diff --git a/lib/recommended-rules.js b/lib/recommended-rules.js index 62d9d0b60b..88306e68ac 100644 --- a/lib/recommended-rules.js +++ b/lib/recommended-rules.js @@ -32,6 +32,7 @@ module.exports = { "ember/no-get-with-default": "error", "ember/no-get": "error", "ember/no-global-jquery": "error", + "ember/no-implicit-injections": "error", "ember/no-incorrect-calls-with-inline-anonymous-functions": "error", "ember/no-incorrect-computed-macros": "error", "ember/no-invalid-debug-function-arguments": "error", diff --git a/lib/rules/no-implicit-injections.js b/lib/rules/no-implicit-injections.js new file mode 100644 index 0000000000..3b194d1366 --- /dev/null +++ b/lib/rules/no-implicit-injections.js @@ -0,0 +1,176 @@ +'use strict'; + +const emberUtils = require('../utils/ember'); +const { getImportIdentifier } = require('../utils/import'); +const Stack = require('../utils/stack'); +const types = require('../utils/types'); +const kebabCase = require('lodash.kebabcase'); +const camelCase = require('lodash.camelcase'); + +const defaultServiceConfig = { serviceName: 'store', moduleNames: ['Route', 'Controller'] }; + +const ERROR_MESSAGE = + 'Do not rely on implicit service injections, these were deprecated in Ember 3.26 and will not work in 4.0.'; + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +function fixService(fixer, currentClass, serviceInjectImportName, servicePropertyName) { + const serviceInjectPath = kebabCase(servicePropertyName); + + return fixer.insertTextBefore( + currentClass.node.body.body[0], + `@${serviceInjectImportName}('${serviceInjectPath}') ${servicePropertyName};\n` + ); +} + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { + meta: { + type: 'suggestion', + docs: { + description: 'enforce usage of implicit service injections', + category: 'Deprecations', + recommended: true, + url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-implicit-injections.md', + }, + fixable: 'code', + schema: [ + { + type: 'object', + properties: { + services: { + type: 'array', + items: { + type: 'object', + default: [{ serviceName: 'store', moduleNames: ['Route', 'Controller'] }], + properties: { + serviceName: { + type: 'string', + }, + moduleNames: { + type: 'array', + items: { + type: 'string', + }, + }, + }, + }, + }, + }, + additionalProperties: false, + }, + ], + }, + + ERROR_MESSAGE, + + create(context) { + const options = context.options[0] || { + services: [defaultServiceConfig], + }; + const servicesConfig = options.services || [defaultServiceConfig]; + + // State being tracked for this file. + let serviceInjectImportName = undefined; + let serviceNamesWithImplicitInjection = servicesConfig; + let isEmberModule = false; + + // State being tracked for the current class we're inside. + const classStack = new Stack(); + + // This rule does not apply to test files or non class modules + if (emberUtils.isTestFile(context.getFilename())) { + return {}; + } + + return { + ImportDeclaration(node) { + if (node.source.value === '@ember/service') { + serviceInjectImportName = + serviceInjectImportName || getImportIdentifier(node, '@ember/service', 'inject'); + } + }, + + ClassDeclaration(node) { + if (emberUtils.isAnyEmberCoreModule(context, node)) { + isEmberModule = true; + + // Get the name of services for the current module type + const serviceNames = servicesConfig + .filter((serviceConfig) => { + return ( + serviceConfig.moduleNames === undefined || + serviceConfig.moduleNames.some((moduleName) => + emberUtils.isEmberCoreModule(context, node, moduleName) + ) + ); + }) + .map((a) => camelCase(a.serviceName)); + + // Get Services that don't have properties/service injections declared + serviceNamesWithImplicitInjection = node.body.body.reduce((accum, n) => { + if (types.isClassPropertyOrPropertyDefinition(n)) { + return accum.filter((serviceName) => !(serviceName === n.key.name)); + } + return accum; + }, serviceNames); + + classStack.push({ + node, + serviceNamesWithImplicitInjection, + }); + } + }, + + 'ClassDeclaration:exit'(node) { + // Leaving current (native) class. + if (classStack.size() > 0 && classStack.peek().node === node) { + classStack.pop(); + } + }, + + MemberExpression(node) { + if (!isEmberModule) { + return; + } + + if (types.isThisExpression(node.object) && types.isIdentifier(node.property)) { + const serviceMissingInjection = serviceNamesWithImplicitInjection.find( + (s) => s === node.property.name + ); + + if (serviceMissingInjection) { + context.report({ + node, + message: ERROR_MESSAGE, + fix(fixer) { + const sourceCode = context.getSourceCode(); + const currentClass = classStack.peek(); + + // service inject is already declared + if (serviceInjectImportName) { + return fixService( + fixer, + currentClass, + serviceInjectImportName, + serviceMissingInjection + ); + } + + return [ + fixer.insertTextBefore( + sourceCode.ast, + "import { inject as service } from '@ember/service';\n" + ), + fixService(fixer, currentClass, 'service', serviceMissingInjection), + ]; + }, + }); + } + } + }, + }; + }, +}; diff --git a/package.json b/package.json index 6c5d3a42e4..4b09f590eb 100644 --- a/package.json +++ b/package.json @@ -72,6 +72,7 @@ "eslint-utils": "^3.0.0", "estraverse": "^5.2.0", "lodash.kebabcase": "^4.1.1", + "lodash.camelcase": "^4.1.1", "magic-string": "^0.27.0", "requireindex": "^1.2.0", "snake-case": "^3.0.3" diff --git a/tests/__snapshots__/recommended.js.snap b/tests/__snapshots__/recommended.js.snap index c52e7bdd9c..2bcbd2fcdd 100644 --- a/tests/__snapshots__/recommended.js.snap +++ b/tests/__snapshots__/recommended.js.snap @@ -29,6 +29,7 @@ exports[`recommended rules has the right list 1`] = ` "no-get-with-default", "no-get", "no-global-jquery", + "no-implicit-injections", "no-incorrect-calls-with-inline-anonymous-functions", "no-incorrect-computed-macros", "no-invalid-debug-function-arguments", diff --git a/tests/lib/rules/no-implicit-injections.js b/tests/lib/rules/no-implicit-injections.js new file mode 100644 index 0000000000..512849475f --- /dev/null +++ b/tests/lib/rules/no-implicit-injections.js @@ -0,0 +1,228 @@ +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require('../../../lib/rules/no-implicit-injections'); +const RuleTester = require('eslint').RuleTester; + +const { ERROR_MESSAGE } = rule; + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const ruleTester = new RuleTester({ + parser: require.resolve('@babel/eslint-parser'), + parserOptions: { + ecmaVersion: 2020, + sourceType: 'module', + babelOptions: { + configFile: require.resolve('../../../.babelrc'), + }, + }, +}); + +ruleTester.run('no-implicit-injections', rule, { + valid: [ + { + filename: 'routes/index.js', + code: ` + import Route from '@ember/routing/route'; + import { inject as service } from '@ember/service'; + + export default class IndexRoute extends Route { + @service('store') store; + async model() { + return this.store.findAll('rental'); + } + }`, + }, + { + filename: 'controller/index.js', + code: ` + import Controller from '@ember/controller'; + import { action } from '@ember/object'; + import { inject as service } from '@ember/service'; + + export default class IndexController extends Controller { + @service('store') store; + @action + async loadUsers() { + return this.store.findAll('user'); + } + }`, + }, + // Only checks for ThisExpression + { + filename: 'controller/index.js', + code: ` + import Controller from '@ember/controller'; + import { action } from '@ember/object'; + + export default class IndexController extends Controller { + @action + async loadUsers(arg) { + return arg.store.findAll('user'); + } + }`, + }, + { + filename: 'components/foobar.js', + code: ` + import Component from '@ember/component'; + + export default class FoobarTest extends Component { + async model() { + return this.store.isXs; + } + }`, + }, + { + filename: 'components/foobar.js', + code: ` + import Component from '@glimmer/component'; + + export default class FoobarTest extends Component { + async model() { + return this.store.isXs; + } + }`, + }, + { + filename: 'routes/index.js', + code: ` + import Route from '@ember/routing/route'; + + export default class IndexRoute extends Route { + get isSmallScreen() { + return this.media.isXs; + } + }`, + options: [ + { + services: [{ serviceName: 'media', moduleNames: ['Component', 'Controller'] }], + }, + ], + }, + { + filename: 'utils/support.js', + code: ` + export function checkMedia() { + return this.media.isXs; + }`, + options: [ + { + services: [{ serviceName: 'media' }], + }, + ], + }, + ], + invalid: [ + { + filename: 'routes/index.js', + code: ` + import Route from '@ember/routing/route'; + + export default class IndexRoute extends Route { + message = 'hello'; + async model() { + return this.store.findAll('rental'); + } + }`, + output: ` + import { inject as service } from '@ember/service'; +import Route from '@ember/routing/route'; + + export default class IndexRoute extends Route { + @service('store') store; +message = 'hello'; + async model() { + return this.store.findAll('rental'); + } + }`, + errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], + }, + { + filename: 'controller/index.js', + code: ` + import Controller from '@ember/controller'; + import { action } from '@ember/object'; + + export default class IndexController extends Controller { + @action + async loadUsers() { + return this.store.findAll('user'); + } + }`, + output: ` + import { inject as service } from '@ember/service'; +import Controller from '@ember/controller'; + import { action } from '@ember/object'; + + export default class IndexController extends Controller { + @service('store') store; +@action + async loadUsers() { + return this.store.findAll('user'); + } + }`, + errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], + }, + { + filename: 'components/foobar.js', + code: ` + import Component from '@ember/component'; + + export default class FoobarTestError extends Component { + get isSmallScreen() { + return this.media.isXs; + } + }`, + output: ` + import { inject as service } from '@ember/service'; +import Component from '@ember/component'; + + export default class FoobarTestError extends Component { + @service('media') media; +get isSmallScreen() { + return this.media.isXs; + } + }`, + options: [ + { + services: [{ serviceName: 'media', moduleNames: ['Component', 'Controller'] }], + }, + ], + errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], + }, + { + filename: 'components/foobar.js', + code: ` + import Component from '@ember/component'; + + export default class FoobarTestError extends Component { + @action + get save() { + return this.flashMessages.warn('some message'); + } + }`, + output: ` + import { inject as service } from '@ember/service'; +import Component from '@ember/component'; + + export default class FoobarTestError extends Component { + @service('flash-messages') flashMessages; +@action + get save() { + return this.flashMessages.warn('some message'); + } + }`, + options: [ + { + services: [{ serviceName: 'flashMessages' }], + }, + ], + errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], + }, + ], +}); From ad910a923ef77663a974df83eee14121b3a99c5b Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Wed, 21 Dec 2022 16:33:06 -0600 Subject: [PATCH 02/22] test: add tests for existing import for inject --- tests/lib/rules/no-implicit-injections.js | 60 +++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/lib/rules/no-implicit-injections.js b/tests/lib/rules/no-implicit-injections.js index 512849475f..51f58575cb 100644 --- a/tests/lib/rules/no-implicit-injections.js +++ b/tests/lib/rules/no-implicit-injections.js @@ -118,6 +118,7 @@ ruleTester.run('no-implicit-injections', rule, { }, ], invalid: [ + // Basic store lint error in routes/controllers { filename: 'routes/index.js', code: ` @@ -168,6 +169,64 @@ import Controller from '@ember/controller'; }`, errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], }, + // Existing import for service injection + { + filename: 'routes/index.js', + code: ` + import { inject as service } from '@ember/service'; + import Route from '@ember/routing/route'; + + export default class IndexRoute extends Route { + @service('router') router; + message = 'hello'; + async model() { + return this.store.findAll('rental'); + } + }`, + output: ` + import { inject as service } from '@ember/service'; + import Route from '@ember/routing/route'; + + export default class IndexRoute extends Route { + @service('store') store; +@service('router') router; + message = 'hello'; + async model() { + return this.store.findAll('rental'); + } + }`, + errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], + }, + + { + filename: 'routes/index.js', + code: ` + import { inject } from '@ember/service'; + import Route from '@ember/routing/route'; + + export default class IndexRoute extends Route { + @inject('router') router; + message = 'hello'; + async model() { + return this.store.findAll('rental'); + } + }`, + output: ` + import { inject } from '@ember/service'; + import Route from '@ember/routing/route'; + + export default class IndexRoute extends Route { + @inject('store') store; +@inject('router') router; + message = 'hello'; + async model() { + return this.store.findAll('rental'); + } + }`, + errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], + }, + + // Custom options { filename: 'components/foobar.js', code: ` @@ -195,6 +254,7 @@ get isSmallScreen() { ], errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], }, + // Custom options with dasherized service name { filename: 'components/foobar.js', code: ` From 1e98255dbc20db37860acadf0e8a77c7e9b52c7b Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Thu, 22 Dec 2022 07:28:59 -0600 Subject: [PATCH 03/22] fix: remove no-implicit-injections from reccomended --- README.md | 2 +- docs/rules/no-implicit-injections.md | 14 ++++---------- lib/recommended-rules.js | 1 - lib/rules/no-implicit-injections.js | 2 +- tests/__snapshots__/recommended.js.snap | 1 - 5 files changed, 6 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index d65e1ca023..b291b05aff 100644 --- a/README.md +++ b/README.md @@ -102,7 +102,7 @@ module.exports = { | [new-module-imports](docs/rules/new-module-imports.md) | enforce using "New Module Imports" from Ember RFC #176 | ✅ | | | | [no-array-prototype-extensions](docs/rules/no-array-prototype-extensions.md) | disallow usage of Ember's `Array` prototype extensions | | 🔧 | | | [no-function-prototype-extensions](docs/rules/no-function-prototype-extensions.md) | disallow usage of Ember's `function` prototype extensions | ✅ | | | -| [no-implicit-injections](docs/rules/no-implicit-injections.md) | enforce usage of implicit service injections | ✅ | 🔧 | | +| [no-implicit-injections](docs/rules/no-implicit-injections.md) | enforce usage of implicit service injections | | 🔧 | | | [no-mixins](docs/rules/no-mixins.md) | disallow the usage of mixins | ✅ | | | | [no-new-mixins](docs/rules/no-new-mixins.md) | disallow the creation of new mixins | ✅ | | | | [no-observers](docs/rules/no-observers.md) | disallow usage of observers | ✅ | | | diff --git a/docs/rules/no-implicit-injections.md b/docs/rules/no-implicit-injections.md index dd8409d050..a9aa964554 100644 --- a/docs/rules/no-implicit-injections.md +++ b/docs/rules/no-implicit-injections.md @@ -1,15 +1,9 @@ # ember/no-implicit-injections -💼 This rule is enabled in the ✅ `recommended` [config](https://github.com/ember-cli/eslint-plugin-ember#-configurations). - 🔧 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix). -✅ The `"extends": "plugin:ember/recommended"` property in a configuration file enables this rule. - -🔧 The `--fix` option on the [command line](https://eslint.org/docs/user-guide/command-line-interface#fixing-problems) can automatically fix some of the problems reported by this rule. - Ember 3.26 introduced a deprecation for relying on implicit service injections or allowing addons to implicitly inject services into all classes of certain types. Support for this is dropped in Ember 4.0. In many applications, `this.store` from Ember Data is often used without injecting the `store` service in Controllers or Routes. Other addons may also have included implicit service injections via initializers and the `application.inject` API. @@ -37,8 +31,8 @@ export default class IndexRoute extends Route { ``` ```js -// controller/index.js -import Route from '@ember/routing/route'; +// controllers/index.js +import Controller from '@ember/controller'; import { action } from '@ember/object'; export default class IndexController extends Controller { @@ -99,14 +93,14 @@ Example config: ```js module.exports = { rules: { - 'ember/no-implicit-injections': { + 'ember/no-implicit-injections': [2, { services: [ // Ember Responsive Used to Auto Inject the media service in Components/Controllers { serviceName: 'media', moduleNames: ['Component', 'Controller'] }, // Ember CLI Flash Used to Auto Inject the flashMessages service in all modules { serviceName: 'flashMessages' }, ] - } + }] } } ``` diff --git a/lib/recommended-rules.js b/lib/recommended-rules.js index 88306e68ac..62d9d0b60b 100644 --- a/lib/recommended-rules.js +++ b/lib/recommended-rules.js @@ -32,7 +32,6 @@ module.exports = { "ember/no-get-with-default": "error", "ember/no-get": "error", "ember/no-global-jquery": "error", - "ember/no-implicit-injections": "error", "ember/no-incorrect-calls-with-inline-anonymous-functions": "error", "ember/no-incorrect-computed-macros": "error", "ember/no-invalid-debug-function-arguments": "error", diff --git a/lib/rules/no-implicit-injections.js b/lib/rules/no-implicit-injections.js index 3b194d1366..1f1f54a2a6 100644 --- a/lib/rules/no-implicit-injections.js +++ b/lib/rules/no-implicit-injections.js @@ -32,7 +32,7 @@ module.exports = { docs: { description: 'enforce usage of implicit service injections', category: 'Deprecations', - recommended: true, + recommended: false, url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-implicit-injections.md', }, fixable: 'code', diff --git a/tests/__snapshots__/recommended.js.snap b/tests/__snapshots__/recommended.js.snap index 2bcbd2fcdd..c52e7bdd9c 100644 --- a/tests/__snapshots__/recommended.js.snap +++ b/tests/__snapshots__/recommended.js.snap @@ -29,7 +29,6 @@ exports[`recommended rules has the right list 1`] = ` "no-get-with-default", "no-get", "no-global-jquery", - "no-implicit-injections", "no-incorrect-calls-with-inline-anonymous-functions", "no-incorrect-computed-macros", "no-invalid-debug-function-arguments", From 86568f8fcfe84c0b40aa8a44608a0a4c938d28b1 Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Tue, 27 Dec 2022 13:26:48 -0600 Subject: [PATCH 04/22] fix: use messageId for more informative lint errors --- lib/rules/no-implicit-injections.js | 13 +++++++------ tests/lib/rules/no-implicit-injections.js | 16 ++++++++-------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/lib/rules/no-implicit-injections.js b/lib/rules/no-implicit-injections.js index 1f1f54a2a6..82b368c9e8 100644 --- a/lib/rules/no-implicit-injections.js +++ b/lib/rules/no-implicit-injections.js @@ -9,9 +9,6 @@ const camelCase = require('lodash.camelcase'); const defaultServiceConfig = { serviceName: 'store', moduleNames: ['Route', 'Controller'] }; -const ERROR_MESSAGE = - 'Do not rely on implicit service injections, these were deprecated in Ember 3.26 and will not work in 4.0.'; - //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -62,10 +59,11 @@ module.exports = { additionalProperties: false, }, ], + messages: { + main: 'Do not rely on implicit service injections for the "{{serviceName}}" service. Implicit service injections were deprecated in Ember 3.26 and will not work in 4.0.', + }, }, - ERROR_MESSAGE, - create(context) { const options = context.options[0] || { services: [defaultServiceConfig], @@ -144,7 +142,10 @@ module.exports = { if (serviceMissingInjection) { context.report({ node, - message: ERROR_MESSAGE, + messageId: 'main', + data: { + serviceName: serviceMissingInjection, + }, fix(fixer) { const sourceCode = context.getSourceCode(); const currentClass = classStack.peek(); diff --git a/tests/lib/rules/no-implicit-injections.js b/tests/lib/rules/no-implicit-injections.js index 51f58575cb..c0bd7fe48e 100644 --- a/tests/lib/rules/no-implicit-injections.js +++ b/tests/lib/rules/no-implicit-injections.js @@ -5,8 +5,6 @@ const rule = require('../../../lib/rules/no-implicit-injections'); const RuleTester = require('eslint').RuleTester; -const { ERROR_MESSAGE } = rule; - //------------------------------------------------------------------------------ // Tests //------------------------------------------------------------------------------ @@ -141,7 +139,7 @@ message = 'hello'; return this.store.findAll('rental'); } }`, - errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], + errors: [{ messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }], }, { filename: 'controller/index.js', @@ -167,7 +165,7 @@ import Controller from '@ember/controller'; return this.store.findAll('user'); } }`, - errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], + errors: [{ messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }], }, // Existing import for service injection { @@ -195,7 +193,7 @@ import Controller from '@ember/controller'; return this.store.findAll('rental'); } }`, - errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], + errors: [{ messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }], }, { @@ -223,7 +221,7 @@ import Controller from '@ember/controller'; return this.store.findAll('rental'); } }`, - errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], + errors: [{ messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }], }, // Custom options @@ -252,7 +250,7 @@ get isSmallScreen() { services: [{ serviceName: 'media', moduleNames: ['Component', 'Controller'] }], }, ], - errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], + errors: [{ messageId: 'main', data: { serviceName: 'media' }, type: 'MemberExpression' }], }, // Custom options with dasherized service name { @@ -282,7 +280,9 @@ import Component from '@ember/component'; services: [{ serviceName: 'flashMessages' }], }, ], - errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }], + errors: [ + { messageId: 'main', data: { serviceName: 'flashMessages' }, type: 'MemberExpression' }, + ], }, ], }); From 62d9876e2eb33bb4b57d8f90aa9820b4a08ce651 Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Tue, 27 Dec 2022 13:32:54 -0600 Subject: [PATCH 05/22] test: add multi class tests --- tests/lib/rules/no-implicit-injections.js | 81 +++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/tests/lib/rules/no-implicit-injections.js b/tests/lib/rules/no-implicit-injections.js index c0bd7fe48e..9ac7acdce7 100644 --- a/tests/lib/rules/no-implicit-injections.js +++ b/tests/lib/rules/no-implicit-injections.js @@ -26,8 +26,16 @@ ruleTester.run('no-implicit-injections', rule, { filename: 'routes/index.js', code: ` import Route from '@ember/routing/route'; + import Controller from '@ember/controller'; import { inject as service } from '@ember/service'; + export class IndexController extends Controller { + @service('store') store; + async loadData() { + return this.store.findAll('rental'); + } + } + export default class IndexRoute extends Route { @service('store') store; async model() { @@ -173,6 +181,7 @@ import Controller from '@ember/controller'; code: ` import { inject as service } from '@ember/service'; import Route from '@ember/routing/route'; + import Component from '@glimmer/component'; export default class IndexRoute extends Route { @service('router') router; @@ -180,10 +189,26 @@ import Controller from '@ember/controller'; async model() { return this.store.findAll('rental'); } + } + + export class IndexRow extends Component { + @service('store') storeService; + + navigate() { + this.store.find(this.args.id); + } + } + + + export class IndexTable extends Component { + loadData() { + this.store.find(this.args.id); + } }`, output: ` import { inject as service } from '@ember/service'; import Route from '@ember/routing/route'; + import Component from '@glimmer/component'; export default class IndexRoute extends Route { @service('store') store; @@ -192,6 +217,21 @@ import Controller from '@ember/controller'; async model() { return this.store.findAll('rental'); } + } + + export class IndexRow extends Component { + @service('store') storeService; + + navigate() { + this.store.find(this.args.id); + } + } + + + export class IndexTable extends Component { + loadData() { + this.store.find(this.args.id); + } }`, errors: [{ messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }], }, @@ -284,5 +324,46 @@ import Component from '@ember/component'; { messageId: 'main', data: { serviceName: 'flashMessages' }, type: 'MemberExpression' }, ], }, + { + filename: 'routes/index.js', + code: ` + import Route from '@ember/routing/route'; + import Controller from '@ember/controller'; + import { inject as service } from '@ember/service'; + + export class IndexController extends Controller { + async loadData() { + return this.store.findAll('rental'); + } + } + + export default class IndexRoute extends Route { + async model() { + return this.store.findAll('rental'); + } + }`, + output: ` + import Route from '@ember/routing/route'; + import Controller from '@ember/controller'; + import { inject as service } from '@ember/service'; + + export class IndexController extends Controller { + @service('store') store; +async loadData() { + return this.store.findAll('rental'); + } + } + + export default class IndexRoute extends Route { + @service('store') store; +async model() { + return this.store.findAll('rental'); + } + }`, + errors: [ + { messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }, + { messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }, + ], + }, ], }); From e8baa1d0716fa4f2951baf3fe466960122ad4ee0 Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Tue, 27 Dec 2022 13:34:57 -0600 Subject: [PATCH 06/22] fix: use relative md paths --- docs/rules/no-implicit-injections.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/rules/no-implicit-injections.md b/docs/rules/no-implicit-injections.md index a9aa964554..d6421d4761 100644 --- a/docs/rules/no-implicit-injections.md +++ b/docs/rules/no-implicit-injections.md @@ -107,8 +107,8 @@ module.exports = { ## Related Rules -- [no-unnecessary-service-injection-argument](https://github.com/ember-cli/eslint-plugin-ember/blob/master/docs/rules/no-unnecessary-service-injection-argument.md) omit service injection argument if possible -- [no-implicit-service-injection-argument](https://github.com/ember-cli/eslint-plugin-ember/blob/master/docs/rules/no-unnecessary-service-injection-argument.md) require the service injection argument for all services (fix output for this rule follows this pattern) +- [no-unnecessary-service-injection-argument](no-unnecessary-service-injection-argument.md) omit service injection argument if possible +- [no-implicit-service-injection-argument](no-unnecessary-service-injection-argument.md) require the service injection argument for all services (fix output for this rule follows this pattern) ## References From 238c37214af38f121dc58db414eaf62d378b420f Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Wed, 28 Dec 2022 08:20:10 -0600 Subject: [PATCH 07/22] fix: lint errors --- docs/rules/no-implicit-injections.md | 8 ++++---- package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/rules/no-implicit-injections.md b/docs/rules/no-implicit-injections.md index d6421d4761..1bdd81ccf1 100644 --- a/docs/rules/no-implicit-injections.md +++ b/docs/rules/no-implicit-injections.md @@ -23,7 +23,7 @@ Examples of **incorrect** code for this rule: import Route from '@ember/routing/route'; export default class IndexRoute extends Route { - async model() { + model() { return this.store.findAll('rental'); } } @@ -37,7 +37,7 @@ import { action } from '@ember/object'; export default class IndexController extends Controller { @action - async loadUsers() { + loadUsers() { return this.store.findAll('user'); } } @@ -53,7 +53,7 @@ import { inject as service } from '@ember/service'; export default class IndexRoute extends Route { @service('store') store; - async model() { + model() { return this.store.findAll('rental'); } } @@ -69,7 +69,7 @@ export default class IndexController extends Controller { @service('store') store; @action - async loadUsers() { + loadUsers() { return this.store.findAll('user'); } } diff --git a/package.json b/package.json index 4b09f590eb..27e347e579 100644 --- a/package.json +++ b/package.json @@ -71,8 +71,8 @@ "ember-template-imports": "^3.1.1", "eslint-utils": "^3.0.0", "estraverse": "^5.2.0", - "lodash.kebabcase": "^4.1.1", "lodash.camelcase": "^4.1.1", + "lodash.kebabcase": "^4.1.1", "magic-string": "^0.27.0", "requireindex": "^1.2.0", "snake-case": "^3.0.3" From 595c5e590eba3cd32f20cb5e0b956464ca211a48 Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Thu, 29 Dec 2022 10:09:00 -0600 Subject: [PATCH 08/22] fix: pr review feedback on documentation and config --- docs/rules/no-implicit-injections.md | 9 +++------ lib/rules/no-implicit-injections.js | 17 +++++++++++++++-- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/docs/rules/no-implicit-injections.md b/docs/rules/no-implicit-injections.md index 1bdd81ccf1..f968892d79 100644 --- a/docs/rules/no-implicit-injections.md +++ b/docs/rules/no-implicit-injections.md @@ -93,23 +93,20 @@ Example config: ```js module.exports = { rules: { - 'ember/no-implicit-injections': [2, { + 'ember/no-implicit-injections': ['error', { services: [ // Ember Responsive Used to Auto Inject the media service in Components/Controllers { serviceName: 'media', moduleNames: ['Component', 'Controller'] }, // Ember CLI Flash Used to Auto Inject the flashMessages service in all modules { serviceName: 'flashMessages' }, + // Check for uses of the store in Routes or Controllers + { serviceName: 'store', moduleNames: ['Route', 'Controller'] }, ] }] } } ``` -## Related Rules - -- [no-unnecessary-service-injection-argument](no-unnecessary-service-injection-argument.md) omit service injection argument if possible -- [no-implicit-service-injection-argument](no-unnecessary-service-injection-argument.md) require the service injection argument for all services (fix output for this rule follows this pattern) - ## References - [Deprecation](https://deprecations.emberjs.com/v3.x/#toc_implicit-injections) diff --git a/lib/rules/no-implicit-injections.js b/lib/rules/no-implicit-injections.js index 82b368c9e8..cf72a8a86d 100644 --- a/lib/rules/no-implicit-injections.js +++ b/lib/rules/no-implicit-injections.js @@ -38,10 +38,11 @@ module.exports = { type: 'object', properties: { services: { + minItems: 1, type: 'array', items: { type: 'object', - default: [{ serviceName: 'store', moduleNames: ['Route', 'Controller'] }], + default: [defaultServiceConfig], properties: { serviceName: { type: 'string', @@ -49,10 +50,22 @@ module.exports = { moduleNames: { type: 'array', items: { - type: 'string', + enum: [ + 'Component', + 'GlimmerComponent', + 'Controller', + 'Mixin', + 'Route', + 'Service', + 'ArrayProxy', + 'ObjectProxy', + 'EmberObject', + 'Helper', + ], }, }, }, + additionalProperties: false, }, }, }, From c1a1cb10540f02459dea1bedde3305883296310b Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Thu, 29 Dec 2022 10:27:07 -0600 Subject: [PATCH 09/22] fix: remove ignore when in test file --- lib/rules/no-implicit-injections.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lib/rules/no-implicit-injections.js b/lib/rules/no-implicit-injections.js index cf72a8a86d..adf04ac87d 100644 --- a/lib/rules/no-implicit-injections.js +++ b/lib/rules/no-implicit-injections.js @@ -91,11 +91,6 @@ module.exports = { // State being tracked for the current class we're inside. const classStack = new Stack(); - // This rule does not apply to test files or non class modules - if (emberUtils.isTestFile(context.getFilename())) { - return {}; - } - return { ImportDeclaration(node) { if (node.source.value === '@ember/service') { From 62ac9423bfd03d4d005774ac5f77774b9c40b5f5 Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Thu, 29 Dec 2022 11:11:23 -0600 Subject: [PATCH 10/22] fix: check validity in nested modules and classes --- lib/rules/no-implicit-injections.js | 88 +++++----- lib/utils/ember.js | 4 +- lib/utils/types.js | 11 ++ tests/lib/rules/no-implicit-injections.js | 203 +++++++++++++++++++++- tests/lib/utils/ember-test.js | 2 +- 5 files changed, 264 insertions(+), 44 deletions(-) diff --git a/lib/rules/no-implicit-injections.js b/lib/rules/no-implicit-injections.js index adf04ac87d..9d50a4ac2d 100644 --- a/lib/rules/no-implicit-injections.js +++ b/lib/rules/no-implicit-injections.js @@ -86,11 +86,52 @@ module.exports = { // State being tracked for this file. let serviceInjectImportName = undefined; let serviceNamesWithImplicitInjection = servicesConfig; - let isEmberModule = false; // State being tracked for the current class we're inside. const classStack = new Stack(); + function onClassEnter(node) { + if (emberUtils.isAnyEmberCoreModule(context, node)) { + // Get the name of services for the current module type + const serviceNames = servicesConfig + .filter((serviceConfig) => { + return ( + serviceConfig.moduleNames === undefined || + serviceConfig.moduleNames.some((moduleName) => + emberUtils.isEmberCoreModule(context, node, moduleName) + ) + ); + }) + .map((a) => camelCase(a.serviceName)); + + // Get Services that don't have properties/service injections declared + serviceNamesWithImplicitInjection = node.body.body.reduce((accum, n) => { + if (types.isClassPropertyOrPropertyDefinition(n)) { + return accum.filter((serviceName) => !(serviceName === n.key.name)); + } + return accum; + }, serviceNames); + + classStack.push({ + node, + isEmberModule: true, + serviceNamesWithImplicitInjection, + }); + } else { + classStack.push({ + node, + isEmberModule: false, + }); + } + } + + function onClassExit(node) { + // Leaving current (native) class. + if (classStack.size() > 0 && classStack.peek().node === node) { + classStack.pop(); + } + } + return { ImportDeclaration(node) { if (node.source.value === '@ember/service') { @@ -99,46 +140,16 @@ module.exports = { } }, - ClassDeclaration(node) { - if (emberUtils.isAnyEmberCoreModule(context, node)) { - isEmberModule = true; - - // Get the name of services for the current module type - const serviceNames = servicesConfig - .filter((serviceConfig) => { - return ( - serviceConfig.moduleNames === undefined || - serviceConfig.moduleNames.some((moduleName) => - emberUtils.isEmberCoreModule(context, node, moduleName) - ) - ); - }) - .map((a) => camelCase(a.serviceName)); - - // Get Services that don't have properties/service injections declared - serviceNamesWithImplicitInjection = node.body.body.reduce((accum, n) => { - if (types.isClassPropertyOrPropertyDefinition(n)) { - return accum.filter((serviceName) => !(serviceName === n.key.name)); - } - return accum; - }, serviceNames); - - classStack.push({ - node, - serviceNamesWithImplicitInjection, - }); - } - }, + ClassDeclaration: onClassEnter, + ClassExpression: onClassEnter, - 'ClassDeclaration:exit'(node) { - // Leaving current (native) class. - if (classStack.size() > 0 && classStack.peek().node === node) { - classStack.pop(); - } - }, + 'ClassDeclaration:exit': onClassExit, + 'ClassExpression:exit': onClassExit, MemberExpression(node) { - if (!isEmberModule) { + const currentClass = classStack.peek(); + + if (!currentClass || !currentClass.isEmberModule) { return; } @@ -156,7 +167,6 @@ module.exports = { }, fix(fixer) { const sourceCode = context.getSourceCode(); - const currentClass = classStack.peek(); // service inject is already declared if (serviceInjectImportName) { diff --git a/lib/utils/ember.js b/lib/utils/ember.js index e2edd762ca..87dcbdb48e 100644 --- a/lib/utils/ember.js +++ b/lib/utils/ember.js @@ -222,7 +222,7 @@ function isEmberCoreModule(context, node, moduleName) { if (types.isCallExpression(node)) { // "classic" class pattern return isClassicEmberCoreModule(node, moduleName, context.getFilename()); - } else if (types.isClassDeclaration(node)) { + } else if (types.isClassDeclaration(node) || types.isClassExpression(node)) { // native classes if ( // class Foo extends Component @@ -251,7 +251,7 @@ function isEmberCoreModule(context, node, moduleName) { } else { assert( false, - 'Function should only be called on a `CallExpression` (classic class) or `ClassDeclaration` (native class)' + 'Function should only be called on a `CallExpression` (classic class) or `ClassDeclaration`/`ClassExpression` (native class)' ); } return false; diff --git a/lib/utils/types.js b/lib/utils/types.js index 3c17df4c91..a4e607558f 100644 --- a/lib/utils/types.js +++ b/lib/utils/types.js @@ -9,6 +9,7 @@ module.exports = { isCallExpression, isCallWithFunctionExpression, isClassDeclaration, + isClassExpression, isClassPropertyOrPropertyDefinition, isCommaToken, isConciseArrowFunctionWithCallExpression, @@ -130,6 +131,16 @@ function isClassDeclaration(node) { return node !== undefined && node.type === 'ClassDeclaration'; } +/** + * Check whether or not a node is a ClassExpression. + * + * @param {Object} node The node to check. + * @returns {boolean} Whether or not the node is a ClassExpression. + */ +function isClassExpression(node) { + return node !== undefined && node.type === 'ClassExpression'; +} + /** * Check whether or not a node is a ClassProperty (ESLint v7) or PropertyDefinition (ESLint v8). * diff --git a/tests/lib/rules/no-implicit-injections.js b/tests/lib/rules/no-implicit-injections.js index 9ac7acdce7..0c5ea99c09 100644 --- a/tests/lib/rules/no-implicit-injections.js +++ b/tests/lib/rules/no-implicit-injections.js @@ -23,7 +23,7 @@ const ruleTester = new RuleTester({ ruleTester.run('no-implicit-injections', rule, { valid: [ { - filename: 'routes/index.js', + filename: 'pods/index.js', code: ` import Route from '@ember/routing/route'; import Controller from '@ember/controller'; @@ -36,7 +36,7 @@ ruleTester.run('no-implicit-injections', rule, { } } - export default class IndexRoute extends Route { + export class IndexRoute extends Route { @service('store') store; async model() { return this.store.findAll('rental'); @@ -122,6 +122,80 @@ ruleTester.run('no-implicit-injections', rule, { }, ], }, + // Works for modules with multiple module types + { + filename: 'pods/user.js', + code: ` + import Route from '@ember/routing/route'; + import Controller from '@ember/controller'; + import { inject as service } from '@ember/service'; + + export class User { + get mediaAccount() { + return this.media.account; + } + } + + export class UserController extends Controller { + @service('media') media; + + get isSmallScreen() { + return this.media.isXs; + } + } + + export class UserRoute extends Route { + get isSmallScreen() { + return this.media.isXs; + } + }`, + options: [ + { + services: [{ serviceName: 'media', moduleNames: ['Component', 'Controller'] }], + }, + ], + }, + + // Reassesses Module Type for Nested Classes + { + filename: 'controllers/register.js', + code: ` + import Controller from '@ember/controller'; + import { inject as service } from '@ember/service'; + + export class RegisterController extends Controller { + @service('store') store; + async loadData() { + return this.store.findAll('rental'); + } + + getMediaUser() { + return class Register { + get storeInfo() { + return this.store.address; + } + } + } + }`, + }, + + // Nested Ember Module Definition (used in some meta programming instances or decorators) + { + filename: 'utils/loads-user-controller.js', + code: ` + import Controller from '@ember/controller'; + import { inject as service } from '@ember/service'; + + export function mediaAwareRoute() { + return class UserController extends Controller { + @inject('store') store; + async loadData() { + return this.store.findAll('rental'); + } + } + }`, + errors: [{ messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }], + }, ], invalid: [ // Basic store lint error in routes/controllers @@ -365,5 +439,130 @@ async model() { { messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }, ], }, + // Works for modules with multiple module types + { + filename: 'pods/user.js', + code: ` + import Route from '@ember/routing/route'; + import Controller from '@ember/controller'; + import { inject as service } from '@ember/service'; + + export class User { + get mediaAccount() { + return this.media.account; + } + } + + export class UserController extends Controller { + get isSmallScreen() { + return this.media.isXs; + } + } + + export class UserRoute extends Route { + get isSmallScreen() { + return this.media.isXs; + } + }`, + output: ` + import Route from '@ember/routing/route'; + import Controller from '@ember/controller'; + import { inject as service } from '@ember/service'; + + export class User { + get mediaAccount() { + return this.media.account; + } + } + + export class UserController extends Controller { + @service('media') media; +get isSmallScreen() { + return this.media.isXs; + } + } + + export class UserRoute extends Route { + get isSmallScreen() { + return this.media.isXs; + } + }`, + options: [ + { + services: [{ serviceName: 'media', moduleNames: ['Component', 'Controller'] }], + }, + ], + errors: [{ messageId: 'main', data: { serviceName: 'media' }, type: 'MemberExpression' }], + }, + + // Reassesses Module Type for Nested Classes + { + filename: 'controllers/register.js', + code: ` + import Controller from '@ember/controller'; + import { inject as service } from '@ember/service'; + + export class RegisterController extends Controller { + async loadData() { + return this.store.findAll('rental'); + } + + getMediaUser() { + return class Register { + get storeInfo() { + return this.store.address; + } + } + } + }`, + output: ` + import Controller from '@ember/controller'; + import { inject as service } from '@ember/service'; + + export class RegisterController extends Controller { + @service('store') store; +async loadData() { + return this.store.findAll('rental'); + } + + getMediaUser() { + return class Register { + get storeInfo() { + return this.store.address; + } + } + } + }`, + errors: [{ messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }], + }, + + // Nested Ember Module Definition (used in some meta programming instances or decorators) + { + filename: 'utils/loads-user-controller.js', + code: ` + import Controller from '@ember/controller'; + import { inject as service } from '@ember/service'; + + export function mediaAwareRoute() { + return class UserController extends Controller { + async loadData() { + return this.store.findAll('rental'); + } + } + }`, + output: ` + import Controller from '@ember/controller'; + import { inject as service } from '@ember/service'; + + export function mediaAwareRoute() { + return class UserController extends Controller { + @service('store') store; +async loadData() { + return this.store.findAll('rental'); + } + } + }`, + errors: [{ messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }], + }, ], }); diff --git a/tests/lib/utils/ember-test.js b/tests/lib/utils/ember-test.js index e53dd8cbc3..9a286131c7 100644 --- a/tests/lib/utils/ember-test.js +++ b/tests/lib/utils/ember-test.js @@ -235,7 +235,7 @@ describe('isEmberCoreModule', () => { const context = new FauxContext('const x = 123;'); const node = context.ast.body[0]; expect(() => emberUtils.isEmberCoreModule(context, node, 'Route')).toThrow( - 'Function should only be called on a `CallExpression` (classic class) or `ClassDeclaration` (native class)' + 'Function should only be called on a `CallExpression` (classic class) or `ClassDeclaration`/`ClassExpression` (native class)' ); }); }); From 2b28afec36f7e13f3e0d4f87dd446e06148f62e5 Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Thu, 29 Dec 2022 12:59:52 -0600 Subject: [PATCH 11/22] fix: config schema changes --- lib/rules/no-implicit-injections.js | 3 +++ yarn.lock | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/rules/no-implicit-injections.js b/lib/rules/no-implicit-injections.js index 9d50a4ac2d..1f7440f18a 100644 --- a/lib/rules/no-implicit-injections.js +++ b/lib/rules/no-implicit-injections.js @@ -46,9 +46,12 @@ module.exports = { properties: { serviceName: { type: 'string', + required: true, + minLength: 1, }, moduleNames: { type: 'array', + required: false, items: { enum: [ 'Component', diff --git a/yarn.lock b/yarn.lock index 562d2d4d0d..fbbc829cfb 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4570,7 +4570,7 @@ lodash._reinterpolate@^3.0.0: resolved "https://registry.yarnpkg.com/lodash._reinterpolate/-/lodash._reinterpolate-3.0.0.tgz#0ccf2d89166af03b3663c796538b75ac6e114d9d" integrity sha512-xYHt68QRoYGjeeM/XOE1uJtvXQAgvszfBhjV4yvsQH0u2i9I6cI6c6/eG4Hh3UAOVn0y/xAXwmTzEay49Q//HA== -lodash.camelcase@4.3.0: +lodash.camelcase@4.3.0, lodash.camelcase@^4.1.1: version "4.3.0" resolved "https://registry.yarnpkg.com/lodash.camelcase/-/lodash.camelcase-4.3.0.tgz#b28aa6288a2b9fc651035c7711f65ab6190331a6" integrity sha512-TwuEnCnxbc3rAvhf/LbG7tJUDzhqXyFnv3dtzLOPgCG/hODL7WFnsbwktkD7yUV0RrreP/l1PALq/YSg6VvjlA== From c81f3f844753c00f57523199c7abe5731c93c86b Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Thu, 29 Dec 2022 13:01:40 -0600 Subject: [PATCH 12/22] doc: add link to ember data store service --- docs/rules/no-implicit-injections.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/rules/no-implicit-injections.md b/docs/rules/no-implicit-injections.md index f968892d79..909d2f0b98 100644 --- a/docs/rules/no-implicit-injections.md +++ b/docs/rules/no-implicit-injections.md @@ -110,3 +110,4 @@ module.exports = { ## References - [Deprecation](https://deprecations.emberjs.com/v3.x/#toc_implicit-injections) +- [Ember Data Store Service](https://api.emberjs.com/ember-data/release/classes/Store) From dadc34ddc8e51b4236777e08170c5230fb799939 Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Fri, 30 Dec 2022 08:55:32 -0600 Subject: [PATCH 13/22] fix: config required properties and kebab case setting --- docs/rules/no-implicit-injections.md | 12 ++++----- lib/rules/no-implicit-injections.js | 31 ++++++++++++++--------- tests/lib/rules/no-implicit-injections.js | 14 +++++----- 3 files changed, 32 insertions(+), 25 deletions(-) diff --git a/docs/rules/no-implicit-injections.md b/docs/rules/no-implicit-injections.md index 909d2f0b98..3efd0a6a0e 100644 --- a/docs/rules/no-implicit-injections.md +++ b/docs/rules/no-implicit-injections.md @@ -84,8 +84,8 @@ The autofixer for this rule will update classes and add injections for the confi This lint rule will search for instances of `store` used in routes or controllers by default. If you have other services that you would like to check for uses of, the configuration can be overridden. - object -- containing the following properties: - - array -- `services` -- Array of configuration objects configuring the lint rule to check for use of implicit injected services - - string -- `serviceName` -- The name of the service that is implicitly injected + - array -- `denyList` -- Array of configuration objects configuring the lint rule to check for use of implicit injected services + - string -- `service` -- The name of the service that is implicitly injected - array -- `moduleNames` -- Array of string listing the types of classes (controller, route, component, etc) to check for implicit injections. If an array is declared only those class types will be checked for implicit injection. (Defaults to checking all class files/types) Example config: @@ -94,13 +94,13 @@ Example config: module.exports = { rules: { 'ember/no-implicit-injections': ['error', { - services: [ + denyList: [ // Ember Responsive Used to Auto Inject the media service in Components/Controllers - { serviceName: 'media', moduleNames: ['Component', 'Controller'] }, + { service: 'media', moduleNames: ['Component', 'Controller'] }, // Ember CLI Flash Used to Auto Inject the flashMessages service in all modules - { serviceName: 'flashMessages' }, + { service: 'flash-messages' }, // Check for uses of the store in Routes or Controllers - { serviceName: 'store', moduleNames: ['Route', 'Controller'] }, + { service: 'store', moduleNames: ['Route', 'Controller'] }, ] }] } diff --git a/lib/rules/no-implicit-injections.js b/lib/rules/no-implicit-injections.js index 1f7440f18a..b855a31d1f 100644 --- a/lib/rules/no-implicit-injections.js +++ b/lib/rules/no-implicit-injections.js @@ -1,5 +1,6 @@ 'use strict'; +const assert = require('assert'); const emberUtils = require('../utils/ember'); const { getImportIdentifier } = require('../utils/import'); const Stack = require('../utils/stack'); @@ -7,9 +8,9 @@ const types = require('../utils/types'); const kebabCase = require('lodash.kebabcase'); const camelCase = require('lodash.camelcase'); -const defaultServiceConfig = { serviceName: 'store', moduleNames: ['Route', 'Controller'] }; +const defaultServiceConfig = { service: 'store', moduleNames: ['Route', 'Controller'] }; -//------------------------------------------------------------------------------ +// ----- ------------------------------------------------------------------------- // Rule Definition //------------------------------------------------------------------------------ @@ -37,21 +38,20 @@ module.exports = { { type: 'object', properties: { - services: { + denyList: { minItems: 1, type: 'array', items: { type: 'object', default: [defaultServiceConfig], + required: ['service'], properties: { - serviceName: { + service: { type: 'string', - required: true, minLength: 1, }, moduleNames: { type: 'array', - required: false, items: { enum: [ 'Component', @@ -82,13 +82,20 @@ module.exports = { create(context) { const options = context.options[0] || { - services: [defaultServiceConfig], + denyList: [defaultServiceConfig], }; - const servicesConfig = options.services || [defaultServiceConfig]; + const denyList = options.denyList || [defaultServiceConfig]; + + for (const config of denyList) { + assert( + config.service.toLowerCase() === config.service, + 'Service name should be passed in kebab-case (all lower case)' + ); + } // State being tracked for this file. let serviceInjectImportName = undefined; - let serviceNamesWithImplicitInjection = servicesConfig; + let serviceNamesWithImplicitInjection = denyList; // State being tracked for the current class we're inside. const classStack = new Stack(); @@ -96,7 +103,7 @@ module.exports = { function onClassEnter(node) { if (emberUtils.isAnyEmberCoreModule(context, node)) { // Get the name of services for the current module type - const serviceNames = servicesConfig + const serviceNames = denyList .filter((serviceConfig) => { return ( serviceConfig.moduleNames === undefined || @@ -105,7 +112,7 @@ module.exports = { ) ); }) - .map((a) => camelCase(a.serviceName)); + .map((a) => camelCase(a.service)); // Get Services that don't have properties/service injections declared serviceNamesWithImplicitInjection = node.body.body.reduce((accum, n) => { @@ -166,7 +173,7 @@ module.exports = { node, messageId: 'main', data: { - serviceName: serviceMissingInjection, + serviceName: kebabCase(serviceMissingInjection), }, fix(fixer) { const sourceCode = context.getSourceCode(); diff --git a/tests/lib/rules/no-implicit-injections.js b/tests/lib/rules/no-implicit-injections.js index 0c5ea99c09..4bd8611afa 100644 --- a/tests/lib/rules/no-implicit-injections.js +++ b/tests/lib/rules/no-implicit-injections.js @@ -106,7 +106,7 @@ ruleTester.run('no-implicit-injections', rule, { }`, options: [ { - services: [{ serviceName: 'media', moduleNames: ['Component', 'Controller'] }], + denyList: [{ service: 'media', moduleNames: ['Component', 'Controller'] }], }, ], }, @@ -118,7 +118,7 @@ ruleTester.run('no-implicit-injections', rule, { }`, options: [ { - services: [{ serviceName: 'media' }], + denyList: [{ service: 'media' }], }, ], }, @@ -151,7 +151,7 @@ ruleTester.run('no-implicit-injections', rule, { }`, options: [ { - services: [{ serviceName: 'media', moduleNames: ['Component', 'Controller'] }], + denyList: [{ service: 'media', moduleNames: ['Component', 'Controller'] }], }, ], }, @@ -361,7 +361,7 @@ get isSmallScreen() { }`, options: [ { - services: [{ serviceName: 'media', moduleNames: ['Component', 'Controller'] }], + denyList: [{ service: 'media', moduleNames: ['Component', 'Controller'] }], }, ], errors: [{ messageId: 'main', data: { serviceName: 'media' }, type: 'MemberExpression' }], @@ -391,11 +391,11 @@ import Component from '@ember/component'; }`, options: [ { - services: [{ serviceName: 'flashMessages' }], + denyList: [{ service: 'flash-messages' }], }, ], errors: [ - { messageId: 'main', data: { serviceName: 'flashMessages' }, type: 'MemberExpression' }, + { messageId: 'main', data: { serviceName: 'flash-messages' }, type: 'MemberExpression' }, ], }, { @@ -489,7 +489,7 @@ get isSmallScreen() { }`, options: [ { - services: [{ serviceName: 'media', moduleNames: ['Component', 'Controller'] }], + denyList: [{ service: 'media', moduleNames: ['Component', 'Controller'] }], }, ], errors: [{ messageId: 'main', data: { serviceName: 'media' }, type: 'MemberExpression' }], From 30aea4e940f880fe23e94c509180f2896f934dda Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Fri, 30 Dec 2022 10:01:56 -0600 Subject: [PATCH 14/22] fix: remove helper for isClassExpression and add test for new use --- lib/utils/ember.js | 2 +- lib/utils/types.js | 11 ----------- tests/lib/utils/ember-test.js | 11 +++++++++++ 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/utils/ember.js b/lib/utils/ember.js index 87dcbdb48e..cbc1249a17 100644 --- a/lib/utils/ember.js +++ b/lib/utils/ember.js @@ -222,7 +222,7 @@ function isEmberCoreModule(context, node, moduleName) { if (types.isCallExpression(node)) { // "classic" class pattern return isClassicEmberCoreModule(node, moduleName, context.getFilename()); - } else if (types.isClassDeclaration(node) || types.isClassExpression(node)) { + } else if (types.isClassDeclaration(node) || node.type === 'ClassExpression') { // native classes if ( // class Foo extends Component diff --git a/lib/utils/types.js b/lib/utils/types.js index a4e607558f..3c17df4c91 100644 --- a/lib/utils/types.js +++ b/lib/utils/types.js @@ -9,7 +9,6 @@ module.exports = { isCallExpression, isCallWithFunctionExpression, isClassDeclaration, - isClassExpression, isClassPropertyOrPropertyDefinition, isCommaToken, isConciseArrowFunctionWithCallExpression, @@ -131,16 +130,6 @@ function isClassDeclaration(node) { return node !== undefined && node.type === 'ClassDeclaration'; } -/** - * Check whether or not a node is a ClassExpression. - * - * @param {Object} node The node to check. - * @returns {boolean} Whether or not the node is a ClassExpression. - */ -function isClassExpression(node) { - return node !== undefined && node.type === 'ClassExpression'; -} - /** * Check whether or not a node is a ClassProperty (ESLint v7) or PropertyDefinition (ESLint v8). * diff --git a/tests/lib/utils/ember-test.js b/tests/lib/utils/ember-test.js index 9a286131c7..a9639215e7 100644 --- a/tests/lib/utils/ember-test.js +++ b/tests/lib/utils/ember-test.js @@ -222,6 +222,17 @@ describe('isEmberCoreModule', () => { expect(emberUtils.isEmberCoreModule(context, node, 'Route')).toBeTruthy(); }); + it('should check core modules from ClassExpressions', () => { + const context = new FauxContext( + `import Route from '@ember/routing/route'; + + (class MyRoute extends Route {})`, + 'example-app/some-twisted-path/some-route.js' + ); + const node = context.ast.body[1].expression; + expect(emberUtils.isEmberCoreModule(context, node, 'Route')).toBeTruthy(); + }); + it('ignores a native class with a non-identifier super class', () => { const context = new FauxContext( 'class MyRoute extends this.ContainerObject {}', From 0ea5d4e2ad6e7f50a5689a93bf6f55ac1014c2d0 Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Fri, 30 Dec 2022 10:28:46 -0600 Subject: [PATCH 15/22] doc: update doc to clarify that service name should be kebab case --- docs/rules/no-implicit-injections.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/no-implicit-injections.md b/docs/rules/no-implicit-injections.md index 3efd0a6a0e..964e134a04 100644 --- a/docs/rules/no-implicit-injections.md +++ b/docs/rules/no-implicit-injections.md @@ -85,7 +85,7 @@ This lint rule will search for instances of `store` used in routes or controller - object -- containing the following properties: - array -- `denyList` -- Array of configuration objects configuring the lint rule to check for use of implicit injected services - - string -- `service` -- The name of the service that is implicitly injected + - string -- `service` -- The (kebab-case) service name that should be checked for implicit injections and error if found - array -- `moduleNames` -- Array of string listing the types of classes (controller, route, component, etc) to check for implicit injections. If an array is declared only those class types will be checked for implicit injection. (Defaults to checking all class files/types) Example config: From 5bf714fccbfabd43d6ba77fd24c4dc126155b5e5 Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Fri, 30 Dec 2022 11:09:31 -0600 Subject: [PATCH 16/22] Update docs/rules/no-implicit-injections.md Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com> --- docs/rules/no-implicit-injections.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/no-implicit-injections.md b/docs/rules/no-implicit-injections.md index 964e134a04..a73da95058 100644 --- a/docs/rules/no-implicit-injections.md +++ b/docs/rules/no-implicit-injections.md @@ -86,7 +86,7 @@ This lint rule will search for instances of `store` used in routes or controller - object -- containing the following properties: - array -- `denyList` -- Array of configuration objects configuring the lint rule to check for use of implicit injected services - string -- `service` -- The (kebab-case) service name that should be checked for implicit injections and error if found - - array -- `moduleNames` -- Array of string listing the types of classes (controller, route, component, etc) to check for implicit injections. If an array is declared only those class types will be checked for implicit injection. (Defaults to checking all class files/types) + - array -- `moduleNames` -- Array of string listing the types of classes (`Controller, `Route`, `Component`, etc) to check for implicit injections. If an array is declared, only those class types will be checked for implicit injection. (Defaults to checking all class files/types) Example config: From 05434d8fe60118e4e05578b7560ef403f9c72573 Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Fri, 30 Dec 2022 11:16:01 -0600 Subject: [PATCH 17/22] require denylist --- lib/rules/no-implicit-injections.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/rules/no-implicit-injections.js b/lib/rules/no-implicit-injections.js index b855a31d1f..5ad5f8d5ce 100644 --- a/lib/rules/no-implicit-injections.js +++ b/lib/rules/no-implicit-injections.js @@ -37,6 +37,7 @@ module.exports = { schema: [ { type: 'object', + required: ['denyList'], properties: { denyList: { minItems: 1, From 89d594078891a933d7fc8255d9c0eec42b83ddfd Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Fri, 30 Dec 2022 12:11:57 -0600 Subject: [PATCH 18/22] fix: check for property getters or existing definitions --- docs/rules/no-implicit-injections.md | 2 +- lib/rules/no-implicit-injections.js | 2 +- tests/lib/rules/no-implicit-injections.js | 63 ++++++++++++++++++++++- 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/docs/rules/no-implicit-injections.md b/docs/rules/no-implicit-injections.md index a73da95058..29996727da 100644 --- a/docs/rules/no-implicit-injections.md +++ b/docs/rules/no-implicit-injections.md @@ -86,7 +86,7 @@ This lint rule will search for instances of `store` used in routes or controller - object -- containing the following properties: - array -- `denyList` -- Array of configuration objects configuring the lint rule to check for use of implicit injected services - string -- `service` -- The (kebab-case) service name that should be checked for implicit injections and error if found - - array -- `moduleNames` -- Array of string listing the types of classes (`Controller, `Route`, `Component`, etc) to check for implicit injections. If an array is declared, only those class types will be checked for implicit injection. (Defaults to checking all class files/types) + - array -- `moduleNames` -- Array of string listing the types of classes (`Controller`, `Route`, `Component`, etc) to check for implicit injections. If an array is declared, only those class types will be checked for implicit injection. (Defaults to checking all class files/types) Example config: diff --git a/lib/rules/no-implicit-injections.js b/lib/rules/no-implicit-injections.js index 5ad5f8d5ce..b466c108b6 100644 --- a/lib/rules/no-implicit-injections.js +++ b/lib/rules/no-implicit-injections.js @@ -117,7 +117,7 @@ module.exports = { // Get Services that don't have properties/service injections declared serviceNamesWithImplicitInjection = node.body.body.reduce((accum, n) => { - if (types.isClassPropertyOrPropertyDefinition(n)) { + if (types.isClassPropertyOrPropertyDefinition(n) || types.isMethodDefinition(n)) { return accum.filter((serviceName) => !(serviceName === n.key.name)); } return accum; diff --git a/tests/lib/rules/no-implicit-injections.js b/tests/lib/rules/no-implicit-injections.js index 4bd8611afa..627baa7dc0 100644 --- a/tests/lib/rules/no-implicit-injections.js +++ b/tests/lib/rules/no-implicit-injections.js @@ -22,6 +22,7 @@ const ruleTester = new RuleTester({ ruleTester.run('no-implicit-injections', rule, { valid: [ + // Basic use in Route and Controller in single file { filename: 'pods/index.js', code: ` @@ -43,6 +44,7 @@ ruleTester.run('no-implicit-injections', rule, { } }`, }, + // Basic use in Controller { filename: 'controller/index.js', code: ` @@ -58,6 +60,42 @@ ruleTester.run('no-implicit-injections', rule, { } }`, }, + // Ignores if some other property getter is defined + { + filename: 'controller/index.js', + code: ` + import Controller from '@ember/controller'; + import { action } from '@ember/object'; + import { inject as service } from '@ember/service'; + import storeMock from './my-mock'; + + export default class IndexController extends Controller { + store = storeMock; + + @action + async loadUsers() { + return this.store.findAll('user'); + } + }`, + }, + // Ignores if some other property getter is defined + { + filename: 'controller/index.js', + code: ` + import Controller from '@ember/controller'; + import { action } from '@ember/object'; + import { inject as service } from '@ember/service'; + + export default class IndexController extends Controller { + get store() { + return {} + } + @action + async loadUsers() { + return this.store.findAll('user'); + } + }`, + }, // Only checks for ThisExpression { filename: 'controller/index.js', @@ -72,6 +110,7 @@ ruleTester.run('no-implicit-injections', rule, { } }`, }, + // Does not check for Store use in Components { filename: 'components/foobar.js', code: ` @@ -83,6 +122,7 @@ ruleTester.run('no-implicit-injections', rule, { } }`, }, + // Does not check for Store use in GlimmerComponents { filename: 'components/foobar.js', code: ` @@ -94,6 +134,26 @@ ruleTester.run('no-implicit-injections', rule, { } }`, }, + // Checks custom config + { + filename: 'controllers/index.js', + code: ` + import Controller from '@ember/controller'; + + export default class IndexController extends Controller { + @service('media') media; + + get isSmallScreen() { + return this.media.isXs; + } + }`, + options: [ + { + denyList: [{ service: 'media', moduleNames: ['Component', 'Controller'] }], + }, + ], + }, + // Can ignore non-matching module types for custom config { filename: 'routes/index.js', code: ` @@ -110,6 +170,7 @@ ruleTester.run('no-implicit-injections', rule, { }, ], }, + // Ignores use outside of classes { filename: 'utils/support.js', code: ` @@ -122,7 +183,7 @@ ruleTester.run('no-implicit-injections', rule, { }, ], }, - // Works for modules with multiple module types + // Custom Configs work with multiple class/module types both matching and not { filename: 'pods/user.js', code: ` From a66d1ea219ecc185b55aa2f225516e7a347ac30c Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Fri, 30 Dec 2022 12:34:25 -0600 Subject: [PATCH 19/22] wip: testing legacy classes --- tests/lib/rules/no-implicit-injections.js | 154 +++++++++++++++++----- 1 file changed, 121 insertions(+), 33 deletions(-) diff --git a/tests/lib/rules/no-implicit-injections.js b/tests/lib/rules/no-implicit-injections.js index 627baa7dc0..66f5fcde66 100644 --- a/tests/lib/rules/no-implicit-injections.js +++ b/tests/lib/rules/no-implicit-injections.js @@ -20,6 +20,53 @@ const ruleTester = new RuleTester({ }, }); +const FLASH_MESSAGES_CONFIG = { + denyList: [{ service: 'flash-messages' }], +}; +const MEDIA_CONFIG = { + denyList: [{ service: 'media', moduleNames: ['Component', 'Controller'] }], +}; + +function createClassUsage(serviceDefinition) { + return { + filename: 'pods/index.js', + code: ` + import { inject as service } from '@ember/service'; + import Component from '@ember/component'; + + export default class FoobarTestError extends Component { + ${serviceDefinition} + + @action + save() { + return this.flashMessages.warn('some message'); + } + }`, + options: [FLASH_MESSAGES_CONFIG], + }; +} + +function createExtendUsage(serviceDefinition) { + return { + filename: 'pods/index.js', + code: ` + import { inject as service } from '@ember/service'; + import Component from '@ember/component'; + + export default Component.extend({ + ${serviceDefinition} + + actions: { + + save() { + return this.flashMessages.warn('some message'); + } + } + });`, + options: [FLASH_MESSAGES_CONFIG], + }; +} + ruleTester.run('no-implicit-injections', rule, { valid: [ // Basic use in Route and Controller in single file @@ -147,11 +194,7 @@ ruleTester.run('no-implicit-injections', rule, { return this.media.isXs; } }`, - options: [ - { - denyList: [{ service: 'media', moduleNames: ['Component', 'Controller'] }], - }, - ], + options: [MEDIA_CONFIG], }, // Can ignore non-matching module types for custom config { @@ -164,11 +207,7 @@ ruleTester.run('no-implicit-injections', rule, { return this.media.isXs; } }`, - options: [ - { - denyList: [{ service: 'media', moduleNames: ['Component', 'Controller'] }], - }, - ], + options: [MEDIA_CONFIG], }, // Ignores use outside of classes { @@ -210,11 +249,7 @@ ruleTester.run('no-implicit-injections', rule, { return this.media.isXs; } }`, - options: [ - { - denyList: [{ service: 'media', moduleNames: ['Component', 'Controller'] }], - }, - ], + options: [MEDIA_CONFIG], }, // Reassesses Module Type for Nested Classes @@ -255,8 +290,17 @@ ruleTester.run('no-implicit-injections', rule, { } } }`, - errors: [{ messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }], }, + + // Exhaustive check for property definition type + createClassUsage('@service flashMessages'), + createClassUsage('@service() flashMessages'), + createClassUsage("@service('flashMessages') flashMessages"), + createClassUsage("@service('flash-messages') flashMessages"), + + createExtendUsage('flashMessages: service(),'), + createExtendUsage("flashMessages: service('flashMessages'),"), + createExtendUsage("flashMessages: service('flash-messages'),"), ], invalid: [ // Basic store lint error in routes/controllers @@ -420,11 +464,7 @@ get isSmallScreen() { return this.media.isXs; } }`, - options: [ - { - denyList: [{ service: 'media', moduleNames: ['Component', 'Controller'] }], - }, - ], + options: [MEDIA_CONFIG], errors: [{ messageId: 'main', data: { serviceName: 'media' }, type: 'MemberExpression' }], }, // Custom options with dasherized service name @@ -435,7 +475,7 @@ get isSmallScreen() { export default class FoobarTestError extends Component { @action - get save() { + save() { return this.flashMessages.warn('some message'); } }`, @@ -446,15 +486,11 @@ import Component from '@ember/component'; export default class FoobarTestError extends Component { @service('flash-messages') flashMessages; @action - get save() { + save() { return this.flashMessages.warn('some message'); } }`, - options: [ - { - denyList: [{ service: 'flash-messages' }], - }, - ], + options: [FLASH_MESSAGES_CONFIG], errors: [ { messageId: 'main', data: { serviceName: 'flash-messages' }, type: 'MemberExpression' }, ], @@ -548,11 +584,7 @@ get isSmallScreen() { return this.media.isXs; } }`, - options: [ - { - denyList: [{ service: 'media', moduleNames: ['Component', 'Controller'] }], - }, - ], + options: [MEDIA_CONFIG], errors: [{ messageId: 'main', data: { serviceName: 'media' }, type: 'MemberExpression' }], }, @@ -625,5 +657,61 @@ async loadData() { }`, errors: [{ messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }], }, + + { + code: ` + import { inject as service } from '@ember/service'; + import Component from '@ember/component'; + + export default class FoobarTestError extends Component { + @action + save() { + return this.flashMessages.warn('some message'); + } + }`, + output: ` + import { inject as service } from '@ember/service'; + import Component from '@ember/component'; + + export default class FoobarTestError extends Component { + @service('flash-messages') flashMessages; +@action + save() { + return this.flashMessages.warn('some message'); + } + }`, + options: [FLASH_MESSAGES_CONFIG], + errors: [ + { messageId: 'main', data: { serviceName: 'flash-messages' }, type: 'MemberExpression' }, + ], + }, + { + filename: 'pods/index.js', + code: ` + import { inject as service } from '@ember/service'; + import Component from '@ember/component'; + + export default Component.extend({ + actions: { + save() { + return this.flashMessages.warn('some message'); + } + } + });`, + output: ` + import { inject as service } from '@ember/service'; + import Component from '@ember/component'; + + export default Component.extend({ + flashMessages: service('flash-messages'), + actions: { + save() { + return this.flashMessages.warn('some message'); + } + } + });`, + options: [FLASH_MESSAGES_CONFIG], + errors: [{ messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }], + }, ], }); From 919ce526862d699da6022884e1958e670f28ec21 Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Fri, 30 Dec 2022 13:43:12 -0600 Subject: [PATCH 20/22] fix: make it work in legacy extends modules --- lib/rules/no-implicit-injections.js | 84 +++++++++++++++-------- tests/lib/rules/no-implicit-injections.js | 6 +- 2 files changed, 59 insertions(+), 31 deletions(-) diff --git a/lib/rules/no-implicit-injections.js b/lib/rules/no-implicit-injections.js index b466c108b6..d4cfaf74d7 100644 --- a/lib/rules/no-implicit-injections.js +++ b/lib/rules/no-implicit-injections.js @@ -17,10 +17,15 @@ const defaultServiceConfig = { service: 'store', moduleNames: ['Route', 'Control function fixService(fixer, currentClass, serviceInjectImportName, servicePropertyName) { const serviceInjectPath = kebabCase(servicePropertyName); - return fixer.insertTextBefore( - currentClass.node.body.body[0], - `@${serviceInjectImportName}('${serviceInjectPath}') ${servicePropertyName};\n` - ); + return currentClass.node.type === 'CallExpression' + ? fixer.insertTextBefore( + currentClass.node.arguments[0].properties[0], + `${servicePropertyName}: ${serviceInjectImportName}('${serviceInjectPath}'),\n` + ) + : fixer.insertTextBefore( + currentClass.node.body.body[0], + `@${serviceInjectImportName}('${serviceInjectPath}') ${servicePropertyName};\n` + ); } /** @type {import('eslint').Rule.RuleModule} */ @@ -101,33 +106,48 @@ module.exports = { // State being tracked for the current class we're inside. const classStack = new Stack(); + // Array of posible types that could declare existing properties on native or legacy modules + const propertyDefintionTypes = new Set([ + 'Property', + 'ClassProperty', + 'PropertyDefinition', + 'MethodDefinition', + ]); + + function onModuleFound(node) { + // Get the name of services for the current module type + const serviceNames = denyList + .filter((serviceConfig) => { + return ( + serviceConfig.moduleNames === undefined || + serviceConfig.moduleNames.some((moduleName) => + emberUtils.isEmberCoreModule(context, node, moduleName) + ) + ); + }) + .map((a) => camelCase(a.service)); + + const modulePropertyDeclarations = + node.type === 'CallExpression' ? node.arguments[0].properties : node.body.body; + + // Get Services that don't have properties/service injections declared + serviceNamesWithImplicitInjection = modulePropertyDeclarations.reduce((accum, n) => { + if (propertyDefintionTypes.has(n.type)) { + return accum.filter((serviceName) => !(serviceName === n.key.name)); + } + return accum; + }, serviceNames); + + classStack.push({ + node, + isEmberModule: true, + serviceNamesWithImplicitInjection, + }); + } + function onClassEnter(node) { if (emberUtils.isAnyEmberCoreModule(context, node)) { - // Get the name of services for the current module type - const serviceNames = denyList - .filter((serviceConfig) => { - return ( - serviceConfig.moduleNames === undefined || - serviceConfig.moduleNames.some((moduleName) => - emberUtils.isEmberCoreModule(context, node, moduleName) - ) - ); - }) - .map((a) => camelCase(a.service)); - - // Get Services that don't have properties/service injections declared - serviceNamesWithImplicitInjection = node.body.body.reduce((accum, n) => { - if (types.isClassPropertyOrPropertyDefinition(n) || types.isMethodDefinition(n)) { - return accum.filter((serviceName) => !(serviceName === n.key.name)); - } - return accum; - }, serviceNames); - - classStack.push({ - node, - isEmberModule: true, - serviceNamesWithImplicitInjection, - }); + onModuleFound(node); } else { classStack.push({ node, @@ -153,9 +173,15 @@ module.exports = { ClassDeclaration: onClassEnter, ClassExpression: onClassEnter, + CallExpression(node) { + if (emberUtils.isAnyEmberCoreModule(context, node)) { + onModuleFound(node); + } + }, 'ClassDeclaration:exit': onClassExit, 'ClassExpression:exit': onClassExit, + 'CallExpression:exit': onClassExit, MemberExpression(node) { const currentClass = classStack.peek(); diff --git a/tests/lib/rules/no-implicit-injections.js b/tests/lib/rules/no-implicit-injections.js index 66f5fcde66..4e87c8f8a9 100644 --- a/tests/lib/rules/no-implicit-injections.js +++ b/tests/lib/rules/no-implicit-injections.js @@ -704,14 +704,16 @@ async loadData() { export default Component.extend({ flashMessages: service('flash-messages'), - actions: { +actions: { save() { return this.flashMessages.warn('some message'); } } });`, options: [FLASH_MESSAGES_CONFIG], - errors: [{ messageId: 'main', data: { serviceName: 'store' }, type: 'MemberExpression' }], + errors: [ + { messageId: 'main', data: { serviceName: 'flash-messages' }, type: 'MemberExpression' }, + ], }, ], }); From 61ebd4f237dea043e9ce63122ae03b63bc4daef6 Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Fri, 30 Dec 2022 14:19:09 -0600 Subject: [PATCH 21/22] feat: allow custom mapping for property name --- docs/rules/no-implicit-injections.md | 5 +- lib/rules/no-implicit-injections.js | 90 ++++++++++++----------- tests/lib/rules/no-implicit-injections.js | 51 ++++++++++++- 3 files changed, 100 insertions(+), 46 deletions(-) diff --git a/docs/rules/no-implicit-injections.md b/docs/rules/no-implicit-injections.md index 29996727da..c9337ee2aa 100644 --- a/docs/rules/no-implicit-injections.md +++ b/docs/rules/no-implicit-injections.md @@ -86,7 +86,8 @@ This lint rule will search for instances of `store` used in routes or controller - object -- containing the following properties: - array -- `denyList` -- Array of configuration objects configuring the lint rule to check for use of implicit injected services - string -- `service` -- The (kebab-case) service name that should be checked for implicit injections and error if found - - array -- `moduleNames` -- Array of string listing the types of classes (`Controller`, `Route`, `Component`, etc) to check for implicit injections. If an array is declared, only those class types will be checked for implicit injection. (Defaults to checking all class files/types) + - array -- `propertyName` -- The property name where the service would be injected in classes. This defaults to the camel case version of the `service` config above. + - array -- `moduleNames` -- Array of string listing the types of classes (`Controller`, `Route`, `Component`, etc) to check for implicit injections. If an array is declared, only those class types will be checked for implicit injection. (Defaults to checking all Ember Module class files/types) Example config: @@ -101,6 +102,8 @@ module.exports = { { service: 'flash-messages' }, // Check for uses of the store in Routes or Controllers { service: 'store', moduleNames: ['Route', 'Controller'] }, + // Check for the feature service injected as "featureChecker" + { service: 'feature', propertyName: 'featureChecker' }, ] }] } diff --git a/lib/rules/no-implicit-injections.js b/lib/rules/no-implicit-injections.js index d4cfaf74d7..0ed8674337 100644 --- a/lib/rules/no-implicit-injections.js +++ b/lib/rules/no-implicit-injections.js @@ -9,25 +9,45 @@ const kebabCase = require('lodash.kebabcase'); const camelCase = require('lodash.camelcase'); const defaultServiceConfig = { service: 'store', moduleNames: ['Route', 'Controller'] }; +const MODULE_TYPES = [ + 'Component', + 'GlimmerComponent', + 'Controller', + 'Mixin', + 'Route', + 'Service', + 'ArrayProxy', + 'ObjectProxy', + 'EmberObject', + 'Helper', +]; // ----- ------------------------------------------------------------------------- // Rule Definition //------------------------------------------------------------------------------ -function fixService(fixer, currentClass, serviceInjectImportName, servicePropertyName) { - const serviceInjectPath = kebabCase(servicePropertyName); +function fixService(fixer, currentClass, serviceInjectImportName, failedConfig) { + const serviceInjectPath = failedConfig.service; return currentClass.node.type === 'CallExpression' ? fixer.insertTextBefore( currentClass.node.arguments[0].properties[0], - `${servicePropertyName}: ${serviceInjectImportName}('${serviceInjectPath}'),\n` + `${failedConfig.propertyName}: ${serviceInjectImportName}('${serviceInjectPath}'),\n` ) : fixer.insertTextBefore( currentClass.node.body.body[0], - `@${serviceInjectImportName}('${serviceInjectPath}') ${servicePropertyName};\n` + `@${serviceInjectImportName}('${serviceInjectPath}') ${failedConfig.propertyName};\n` ); } +function normalizeConfiguration(denyList) { + return denyList.map((config) => ({ + service: config.service, + propertyName: config.propertyName ?? camelCase(config.service), + moduleNames: config.moduleNames ?? MODULE_TYPES, + })); +} + /** @type {import('eslint').Rule.RuleModule} */ module.exports = { meta: { @@ -56,21 +76,14 @@ module.exports = { type: 'string', minLength: 1, }, + propertyName: { + type: 'string', + minLength: 1, + }, moduleNames: { type: 'array', items: { - enum: [ - 'Component', - 'GlimmerComponent', - 'Controller', - 'Mixin', - 'Route', - 'Service', - 'ArrayProxy', - 'ObjectProxy', - 'EmberObject', - 'Helper', - ], + enum: MODULE_TYPES, }, }, }, @@ -101,7 +114,7 @@ module.exports = { // State being tracked for this file. let serviceInjectImportName = undefined; - let serviceNamesWithImplicitInjection = denyList; + const normalizedConfiguration = normalizeConfiguration(denyList); // State being tracked for the current class we're inside. const classStack = new Stack(); @@ -116,32 +129,30 @@ module.exports = { function onModuleFound(node) { // Get the name of services for the current module type - const serviceNames = denyList - .filter((serviceConfig) => { - return ( - serviceConfig.moduleNames === undefined || - serviceConfig.moduleNames.some((moduleName) => - emberUtils.isEmberCoreModule(context, node, moduleName) - ) - ); - }) - .map((a) => camelCase(a.service)); + let configToCheckFor = normalizedConfiguration.filter((serviceConfig) => { + return ( + serviceConfig.moduleNames === undefined || + serviceConfig.moduleNames.some((moduleName) => + emberUtils.isEmberCoreModule(context, node, moduleName) + ) + ); + }); const modulePropertyDeclarations = node.type === 'CallExpression' ? node.arguments[0].properties : node.body.body; // Get Services that don't have properties/service injections declared - serviceNamesWithImplicitInjection = modulePropertyDeclarations.reduce((accum, n) => { + configToCheckFor = modulePropertyDeclarations.reduce((accum, n) => { if (propertyDefintionTypes.has(n.type)) { - return accum.filter((serviceName) => !(serviceName === n.key.name)); + return accum.filter((config) => !(config.propertyName === n.key.name)); } return accum; - }, serviceNames); + }, configToCheckFor); classStack.push({ node, isEmberModule: true, - serviceNamesWithImplicitInjection, + configToCheckFor, }); } @@ -191,28 +202,23 @@ module.exports = { } if (types.isThisExpression(node.object) && types.isIdentifier(node.property)) { - const serviceMissingInjection = serviceNamesWithImplicitInjection.find( - (s) => s === node.property.name + const failedConfig = currentClass.configToCheckFor.find( + (s) => s.propertyName === node.property.name ); - if (serviceMissingInjection) { + if (failedConfig) { context.report({ node, messageId: 'main', data: { - serviceName: kebabCase(serviceMissingInjection), + serviceName: kebabCase(failedConfig.service), }, fix(fixer) { const sourceCode = context.getSourceCode(); // service inject is already declared if (serviceInjectImportName) { - return fixService( - fixer, - currentClass, - serviceInjectImportName, - serviceMissingInjection - ); + return fixService(fixer, currentClass, serviceInjectImportName, failedConfig); } return [ @@ -220,7 +226,7 @@ module.exports = { sourceCode.ast, "import { inject as service } from '@ember/service';\n" ), - fixService(fixer, currentClass, 'service', serviceMissingInjection), + fixService(fixer, currentClass, 'service', failedConfig), ]; }, }); diff --git a/tests/lib/rules/no-implicit-injections.js b/tests/lib/rules/no-implicit-injections.js index 4e87c8f8a9..58459cc171 100644 --- a/tests/lib/rules/no-implicit-injections.js +++ b/tests/lib/rules/no-implicit-injections.js @@ -26,6 +26,9 @@ const FLASH_MESSAGES_CONFIG = { const MEDIA_CONFIG = { denyList: [{ service: 'media', moduleNames: ['Component', 'Controller'] }], }; +const FEATURE_CHECKER_CONFIG = { + denyList: [{ service: 'feature', propertyName: 'featureChecker' }], +}; function createClassUsage(serviceDefinition) { return { @@ -196,15 +199,30 @@ ruleTester.run('no-implicit-injections', rule, { }`, options: [MEDIA_CONFIG], }, - // Can ignore non-matching module types for custom config + // Ignores use when property name is modified from 1:1 mapping + { + filename: 'controllers/index.js', + code: ` + import Controller from '@ember/controller'; + + export default class IndexController extends Controller { + get isSmallScreen() { + return this.feature.isXs; + } + }`, + options: [FEATURE_CHECKER_CONFIG], + }, + // Can detect changed property mapping { filename: 'routes/index.js', code: ` import Route from '@ember/routing/route'; export default class IndexRoute extends Route { - get isSmallScreen() { - return this.media.isXs; + featureChecker = null; + + get canVisitCheckout() { + return this.featureChecker.isEnabled('checkout'); } }`, options: [MEDIA_CONFIG], @@ -685,6 +703,33 @@ async loadData() { { messageId: 'main', data: { serviceName: 'flash-messages' }, type: 'MemberExpression' }, ], }, + // Can detect changed property mapping + { + filename: 'routes/checkout.js', + code: ` + import { inject as service } from '@ember/service'; + import Route from '@ember/routing/route'; + + export default class CheckoutRoute extends Route { + get canVisitCheckout() { + return this.featureChecker.isEnabled('checkout'); + } + }`, + output: ` + import { inject as service } from '@ember/service'; + import Route from '@ember/routing/route'; + + export default class CheckoutRoute extends Route { + @service('feature') featureChecker; +get canVisitCheckout() { + return this.featureChecker.isEnabled('checkout'); + } + }`, + options: [FEATURE_CHECKER_CONFIG], + errors: [{ messageId: 'main', data: { serviceName: 'feature' }, type: 'MemberExpression' }], + }, + + // Check use and fix in legacy ember components { filename: 'pods/index.js', code: ` From 23b0e1abf1ad75b6a9577c8af0fdb06f7a2052f9 Mon Sep 17 00:00:00 2001 From: Ryan Tablada Date: Fri, 30 Dec 2022 14:31:33 -0600 Subject: [PATCH 22/22] fix: check for nested services and nested service config --- lib/rules/no-implicit-injections.js | 10 +++-- tests/lib/rules/no-implicit-injections.js | 52 ++++++++++++++++++++++- 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/lib/rules/no-implicit-injections.js b/lib/rules/no-implicit-injections.js index 0ed8674337..b9df3d138e 100644 --- a/lib/rules/no-implicit-injections.js +++ b/lib/rules/no-implicit-injections.js @@ -5,7 +5,6 @@ const emberUtils = require('../utils/ember'); const { getImportIdentifier } = require('../utils/import'); const Stack = require('../utils/stack'); const types = require('../utils/types'); -const kebabCase = require('lodash.kebabcase'); const camelCase = require('lodash.camelcase'); const defaultServiceConfig = { service: 'store', moduleNames: ['Route', 'Controller'] }; @@ -107,9 +106,14 @@ module.exports = { for (const config of denyList) { assert( - config.service.toLowerCase() === config.service, + emberUtils.convertServiceNameToKebabCase(config.service) === config.service, 'Service name should be passed in kebab-case (all lower case)' ); + + assert( + !config.service.includes('/') || config.propertyName, + 'Nested services must declare a property name' + ); } // State being tracked for this file. @@ -211,7 +215,7 @@ module.exports = { node, messageId: 'main', data: { - serviceName: kebabCase(failedConfig.service), + serviceName: failedConfig.service, }, fix(fixer) { const sourceCode = context.getSourceCode(); diff --git a/tests/lib/rules/no-implicit-injections.js b/tests/lib/rules/no-implicit-injections.js index 58459cc171..e0e180b94a 100644 --- a/tests/lib/rules/no-implicit-injections.js +++ b/tests/lib/rules/no-implicit-injections.js @@ -29,6 +29,9 @@ const MEDIA_CONFIG = { const FEATURE_CHECKER_CONFIG = { denyList: [{ service: 'feature', propertyName: 'featureChecker' }], }; +const NESTED_SERVICE_CONFIG = { + denyList: [{ service: 'cart/checkout', propertyName: 'checkout' }], +}; function createClassUsage(serviceDefinition) { return { @@ -216,16 +219,33 @@ ruleTester.run('no-implicit-injections', rule, { { filename: 'routes/index.js', code: ` + import { inject as service } from '@ember/service'; import Route from '@ember/routing/route'; export default class IndexRoute extends Route { - featureChecker = null; + @service('feature') featureChecker; get canVisitCheckout() { return this.featureChecker.isEnabled('checkout'); } }`, - options: [MEDIA_CONFIG], + options: [FEATURE_CHECKER_CONFIG], + }, + // Can work with services with nested module paths + { + filename: 'routes/index.js', + code: ` + import Route from '@ember/routing/route'; + import { inject as service } from '@ember/service'; + + export default class IndexRoute extends Route { + @service('cart/checkout') checkout; + + model() { + return this.checkout.viewCart(); + } + }`, + options: [NESTED_SERVICE_CONFIG], }, // Ignores use outside of classes { @@ -729,6 +749,34 @@ get canVisitCheckout() { errors: [{ messageId: 'main', data: { serviceName: 'feature' }, type: 'MemberExpression' }], }, + // Can work with services with nested module paths + { + filename: 'routes/index.js', + code: ` + import Route from '@ember/routing/route'; + import { inject as service } from '@ember/service'; + + export default class IndexRoute extends Route { + model() { + return this.checkout.viewCart(); + } + }`, + output: ` + import Route from '@ember/routing/route'; + import { inject as service } from '@ember/service'; + + export default class IndexRoute extends Route { + @service('cart/checkout') checkout; +model() { + return this.checkout.viewCart(); + } + }`, + options: [NESTED_SERVICE_CONFIG], + errors: [ + { messageId: 'main', data: { serviceName: 'cart/checkout' }, type: 'MemberExpression' }, + ], + }, + // Check use and fix in legacy ember components { filename: 'pods/index.js',