Merge pull request #1188 from bmish/no-implicit-service-injection-arg…

…ument

README.md

@@ -175,6 +175,7 @@ Rules are grouped by category to help you understand their purpose. Each rule ha
 
 | Name    | Description | :white_check_mark: | :wrench: |
 |:--------|:------------|:---------------|:-----------|
+| [no-implicit-service-injection-argument](./docs/rules/no-implicit-service-injection-argument.md) | disallow omitting the injected service name argument |  | :wrench: |
 | [no-restricted-service-injections](./docs/rules/no-restricted-service-injections.md) | disallow injecting certain services under certain paths |  |  |
 | [no-unnecessary-service-injection-argument](./docs/rules/no-unnecessary-service-injection-argument.md) | disallow unnecessary argument when injecting services |  | :wrench: |
 | [no-unused-services](./docs/rules/no-unused-services.md) | disallow unused service injections (see rule doc for limitations) |  |  |

docs/rules/no-implicit-service-injection-argument.md

@@ -0,0 +1,42 @@
+# no-implicit-service-injection-argument
+
+:wrench: 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.
+
+This rule disallows omitting the service name argument when injecting a service. Instead, the filename of the service should be used (i.e. `service-name` when the service lives at `app/services/service-name.js`).
+
+Some developers may prefer to be more explicit about what service is being injected instead of relying on the implicit and potentially costly lookup/normalization of the service from the property name.
+
+Note: this rule is not in the `recommended` configuration because it is somewhat of a stylistic preference and it's not always necessary to explicitly include the service injection argument.
+
+## Examples
+
+Examples of **incorrect** code for this rule:
+
+```js
+import Component from '@ember/component';
+import { inject as service } from '@ember/service';
+
+export default class Page extends Component {
+  @service() serviceName;
+}
+```
+
+Examples of **correct** code for this rule:
+
+```js
+import Component from '@ember/component';
+import { inject as service } from '@ember/service';
+
+export default class Page extends Component {
+  @service('service-name') serviceName;
+}
+```
+
+## 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) is the opposite of this rule
+
+## References
+
+* Ember [Services](https://guides.emberjs.com/release/applications/services/) guide
+* Ember [inject](https://emberjs.com/api/ember/release/functions/@ember%2Fservice/inject) function spec

docs/rules/no-unnecessary-service-injection-argument.md

@@ -41,6 +41,10 @@ export default Component.extend({
 });
 ```
 
+## Related Rules
+
+* [no-implicit-service-injection-argument](https://github.com/ember-cli/eslint-plugin-ember/blob/master/docs/rules/no-implicit-service-injection-argument.md) is the opposite of this rule
+
 ## References
 
 * Ember [Services](https://guides.emberjs.com/release/applications/services/) guide

lib/rules/no-implicit-service-injection-argument.js

@@ -0,0 +1,116 @@
+'use strict';
+
+const types = require('../utils/types');
+const emberUtils = require('../utils/ember');
+const { getImportIdentifier } = require('../utils/import');
+
+//------------------------------------------------------------------------------
+// Rule Definition
+//------------------------------------------------------------------------------
+
+const ERROR_MESSAGE = "Don't omit the argument for the injected service name.";
+
+module.exports = {
+  meta: {
+    type: 'suggestion',
+    docs: {
+      description: 'disallow omitting the injected service name argument',
+      category: 'Services',
+      recommended: false,
+      url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-implicit-service-injection-argument.md',
+    },
+    fixable: 'code',
+    schema: [],
+  },
+
+  ERROR_MESSAGE,
+
+  create(context) {
+    let importedInjectName;
+    let importedEmberName;
+
+    return {
+      ImportDeclaration(node) {
+        if (node.source.value === 'ember') {
+          importedEmberName = importedEmberName || getImportIdentifier(node, 'ember');
+        }
+        if (node.source.value === '@ember/service') {
+          importedInjectName =
+            importedInjectName || getImportIdentifier(node, '@ember/service', 'inject');
+        }
+      },
+      Property(node) {
+        // Classic classes.
+
+        if (
+          !emberUtils.isInjectedServiceProp(node, importedEmberName, importedInjectName) ||
+          node.value.arguments.length > 0
+        ) {
+          // Already has the service name argument.
+          return;
+        }
+
+        if (node.value.arguments.length === 0) {
+          context.report({
+            node: node.value,
+            message: ERROR_MESSAGE,
+            fix(fixer) {
+              const sourceCode = context.getSourceCode();
+
+              // Ideally, we want to match the service's filename, and kebab-case filenames are most common.
+              const serviceName = emberUtils.convertServiceNameToKebabCase(
+                node.key.name || node.key.value
+              );
+
+              return fixer.insertTextAfter(
+                sourceCode.getTokenAfter(node.value.callee),
+                `'${serviceName}'`
+              );
+            },
+          });
+        }
+      },
+
+      ClassProperty(node) {
+        // Native classes.
+
+        if (
+          !emberUtils.isInjectedServiceProp(node, importedEmberName, importedInjectName) ||
+          node.decorators.length !== 1
+        ) {
+          return;
+        }
+
+        if (
+          types.isCallExpression(node.decorators[0].expression) &&
+          node.decorators[0].expression.arguments.length > 0
+        ) {
+          // Already has the service name argument.
+          return;
+        }
+
+        context.report({
+          node: node.decorators[0].expression,
+          message: ERROR_MESSAGE,
+          fix(fixer) {
+            const sourceCode = context.getSourceCode();
+
+            // Ideally, we want to match the service's filename, and kebab-case filenames are most common.
+            const serviceName = emberUtils.convertServiceNameToKebabCase(
+              node.key.name || node.key.value
+            );
+
+            return node.decorators[0].expression.type === 'CallExpression'
+              ? // Add after parenthesis.
+                fixer.insertTextAfter(
+                  sourceCode.getTokenAfter(node.decorators[0].expression.callee),
+                  `'${serviceName}'`
+                )
+              : // No parenthesis yet so we need to add them.
+                fixer.insertTextAfter(node.decorators[0].expression, `('${serviceName}')`);
+          },
+        });
+      },
+    };
+  },
+};

lib/rules/no-restricted-service-injections.js

@@ -1,6 +1,5 @@
 'use strict';
 
-const kebabCase = require('lodash.kebabcase');
 const assert = require('assert');
 const emberUtils = require('../utils/ember');
 const decoratorUtils = require('../utils/decorators');
@@ -74,7 +73,7 @@ module.exports = {
     }
 
     function checkForViolationAndReport(node, serviceName) {
-      const serviceNameKebabCase = serviceName.split('/').map(kebabCase).join('/'); // splitting is used to avoid converting folder/ to folder-
+      const serviceNameKebabCase = emberUtils.convertServiceNameToKebabCase(serviceName); // splitting is used to avoid converting folder/ to folder-
 
       for (const denylist of denylists) {
         // Denylist services are always passed in in kebab-case, so we can do a kebab-case comparison.

lib/utils/ember.js

@@ -8,6 +8,7 @@ const assert = require('assert');
 const decoratorUtils = require('../utils/decorators');
 const { getNodeOrNodeFromVariable } = require('../utils/utils');
 const { flatMap } = require('../utils/javascript');
+const kebabCase = require('lodash.kebabcase');
 
 module.exports = {
   isDSModel,
@@ -68,6 +69,8 @@ module.exports = {
   isEmberObjectImplementingUnknownProperty,
 
   isObserverDecorator,
+
+  convertServiceNameToKebabCase,
 };
 
 // Private
@@ -765,3 +768,7 @@ function isObserverDecorator(node, importedObservesName) {
     node.expression.callee.name === importedObservesName
   );
 }
+
+function convertServiceNameToKebabCase(serviceName) {
+  return serviceName.split('/').map(kebabCase).join('/'); // splitting is used to avoid converting folder/ to folder-
+}

tests/lib/rules/no-implicit-service-injection-argument.js

@@ -0,0 +1,98 @@
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+const rule = require('../../../lib/rules/no-implicit-service-injection-argument');
+const RuleTester = require('eslint').RuleTester;
+
+const { ERROR_MESSAGE } = rule;
+
+const EMBER_IMPORT = "import Ember from 'ember';";
+const INJECT_IMPORT = "import {inject} from '@ember/service';";
+const SERVICE_IMPORT = "import {inject as service} from '@ember/service';";
+
+//------------------------------------------------------------------------------
+// Tests
+//------------------------------------------------------------------------------
+
+const ruleTester = new RuleTester({
+  parserOptions: {
+    ecmaVersion: 6,
+    sourceType: 'module',
+    ecmaFeatures: { legacyDecorators: true },
+  },
+  parser: require.resolve('@babel/eslint-parser'),
+});
+
+ruleTester.run('no-implicit-service-injection-argument', rule, {
+  valid: [
+    // With argument (classic class):
+    `${EMBER_IMPORT} export default Component.extend({ serviceName: Ember.inject.service('serviceName') });`,
+    `${INJECT_IMPORT} export default Component.extend({ serviceName: inject('serviceName') });`,
+    `${SERVICE_IMPORT} export default Component.extend({ serviceName: service('serviceName') });`,
+    `${SERVICE_IMPORT} export default Component.extend({ serviceName: service('service-name') });`,
+    `${SERVICE_IMPORT} export default Component.extend({ serviceName: service('random') });`,
+    `${SERVICE_IMPORT} export default Component.extend({ serviceName: service(\`service-name\`) });`,
+
+    // With argument (native class)
+    `${SERVICE_IMPORT} class Test { @service('service-name') serviceName }`,
+
+    // Not Ember's `service()` function (classic class):
+    'export default Component.extend({ serviceName: otherFunction() });',
+    `${SERVICE_IMPORT} export default Component.extend({ serviceName: service.foo() });`,
+
+    // Not Ember's `service()` function (native class):
+    `${SERVICE_IMPORT} class Test { @otherDecorator() name }`,
+    `${SERVICE_IMPORT} class Test { @service.foo() name }`,
+    `${SERVICE_IMPORT} class Test { @foo.service() name }`,
+
+    // Spread syntax
+    'export default Component.extend({ ...foo });',
+  ],
+  invalid: [
+    // Classic class
+    {
+      // `service` import
+      code: `${SERVICE_IMPORT} export default Component.extend({ serviceName: service() });`,
+      output: `${SERVICE_IMPORT} export default Component.extend({ serviceName: service('service-name') });`,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+    {
+      // `inject` import
+      code: `${INJECT_IMPORT} export default Component.extend({ serviceName: inject() });`,
+      output: `${INJECT_IMPORT} export default Component.extend({ serviceName: inject('service-name') });`,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+    {
+      // Property name in string literal.
+      code: `${SERVICE_IMPORT} export default Component.extend({ 'serviceName': service() });`,
+      output: `${SERVICE_IMPORT} export default Component.extend({ 'serviceName': service('service-name') });`,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+
+    // Decorator:
+    {
+      code: `${SERVICE_IMPORT} class Test { @service() serviceName }`,
+      output: `${SERVICE_IMPORT} class Test { @service('service-name') serviceName }`,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+    {
+      // Decorator with no parenthesis
+      code: `${SERVICE_IMPORT} class Test { @service serviceName }`,
+      output: `${SERVICE_IMPORT} class Test { @service('service-name') serviceName }`,
+      errors: [{ message: ERROR_MESSAGE, type: 'Identifier' }],
+    },
+    {
+      // No normalization needed.
+      code: `${SERVICE_IMPORT} class Test { @service() foo }`,
+      output: `${SERVICE_IMPORT} class Test { @service('foo') foo }`,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+    {
+      // Scoped/nested service name with property name in string literal.
+      code: `${SERVICE_IMPORT} class Test { @service() 'myScope/myService' }`,
+      output: `${SERVICE_IMPORT} class Test { @service('my-scope/my-service') 'myScope/myService' }`,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+  ],
+});