Merge pull request #1212 from bmish/property-string-literal-value

Improve detection of property names (check string literals in addition to identifiers) in several rules
ember-cli · May 30, 2021 · 1f69e6a · 1f69e6a
2 parents 10ac49f + dc2b12e
commit 1f69e6a
Show file tree

Hide file tree

Showing 12 changed files with 92 additions and 69 deletions.
diff --git a/lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js b/lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js
@@ -59,9 +59,9 @@ function findTrackedProperties(nodeClassDeclaration, trackedImportName) {
         (node) =>
           types.isClassProperty(node) &&
           decoratorUtils.hasDecorator(node, trackedImportName) &&
-          types.isIdentifier(node.key)
+          (types.isIdentifier(node.key) || types.isStringLiteral(node.key))
       )
-      .map((node) => node.key.name)
+      .map((node) => node.key.name || node.key.value)
   );
 }
 

diff --git a/lib/rules/no-controllers.js b/lib/rules/no-controllers.js
@@ -53,8 +53,8 @@ function classDeclarationHasProperty(classDeclaration, propertyName) {
     classDeclaration.body.body.some(
       (item) =>
         types.isClassProperty(item) &&
-        types.isIdentifier(item.key) &&
-        item.key.name === propertyName
+        ((types.isIdentifier(item.key) && item.key.name === propertyName) ||
+          (types.isStringLiteral(item.key) && item.key.value === propertyName))
     )
   );
 }
@@ -69,7 +69,9 @@ function callExpressionClassHasProperty(callExpression, propertyName, scopeManag
         resultingNode &&
         resultingNode.type === 'ObjectExpression' &&
         resultingNode.properties.some(
-          (prop) => types.isIdentifier(prop.key) && prop.key.name === propertyName
+          (prop) =>
+            (types.isIdentifier(prop.key) && prop.key.name === propertyName) ||
+            (types.isStringLiteral(prop.key) && prop.key.value === propertyName)
         )
       );
     })

diff --git a/lib/rules/no-restricted-service-injections.js b/lib/rules/no-restricted-service-injections.js
@@ -123,7 +123,7 @@ module.exports = {
           }
         } else {
           // The service name is the property name.
-          checkForViolationAndReport(node, node.key.name);
+          checkForViolationAndReport(node, node.key.name || node.key.value);
         }
       },
 
@@ -147,7 +147,7 @@ module.exports = {
           serviceDecorator.expression.arguments[0].type === 'Literal' &&
           typeof serviceDecorator.expression.arguments[0].value === 'string'
             ? serviceDecorator.expression.arguments[0].value
-            : node.key.name;
+            : node.key.name || node.key.value;
 
         checkForViolationAndReport(node, serviceName);
       },

diff --git a/lib/rules/no-unnecessary-service-injection-argument.js b/lib/rules/no-unnecessary-service-injection-argument.js
@@ -49,7 +49,7 @@ module.exports = {
           return;
         }
 
-        const keyName = node.key.name;
+        const keyName = node.key.name || node.key.value;
         const firstArg = node.value.arguments[0];
         const firstArgValue = firstArg.value;
         if (keyName === firstArgValue) {
@@ -74,7 +74,7 @@ module.exports = {
           return;
         }
 
-        const keyName = node.key.name;
+        const keyName = node.key.name || node.key.value;
         const firstArg = node.decorators[0].expression.arguments[0];
         const firstArgValue = firstArg.value;
         if (keyName === firstArgValue) {

diff --git a/lib/rules/no-unused-services.js b/lib/rules/no-unused-services.js
@@ -321,8 +321,13 @@ module.exports = {
           currentClass &&
           emberUtils.isInjectedServiceProp(node, importedEmberName, importedInjectName)
         ) {
-          const name = node.key.name;
-          currentClass.services[name] = node;
+          if (node.key.type === 'Identifier') {
+            const name = node.key.name;
+            currentClass.services[name] = node;
+          } else if (types.isStringLiteral(node.key)) {
+            const name = node.key.value;
+            currentClass.services[name] = node;
+          }
         }
       },
       // @service(...) foo;
@@ -337,8 +342,13 @@ module.exports = {
           currentClass &&
           emberUtils.isInjectedServiceProp(node, importedEmberName, importedInjectName)
         ) {
-          const name = node.key.name;
-          currentClass.services[name] = node;
+          if (node.key.type === 'Identifier') {
+            const name = node.key.name;
+            currentClass.services[name] = node;
+          } else if (types.isStringLiteral(node.key)) {
+            const name = node.key.value;
+            currentClass.services[name] = node;
+          }
         }
       },
       // this.foo...

diff --git a/lib/rules/require-computed-property-dependencies.js b/lib/rules/require-computed-property-dependencies.js
@@ -153,10 +153,13 @@ function findInjectedServiceNames(node) {
       }
       if (
         (types.isProperty(child) || types.isClassProperty(child)) &&
-        emberUtils.isInjectedServiceProp(child, importedEmberName, importedInjectName) &&
-        types.isIdentifier(child.key)
+        emberUtils.isInjectedServiceProp(child, importedEmberName, importedInjectName)
       ) {
-        results.push(child.key.name);
+        if (types.isIdentifier(child.key)) {
+          results.push(child.key.name);
+        } else if (types.isStringLiteral(child.key)) {
+          results.push(child.key.value);
+        }
       }
     },
   });

diff --git a/tests/lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js b/tests/lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js
@@ -54,6 +54,19 @@ ruleTester.run('no-assignment-of-untracked-properties-used-in-tracking-contexts'
       }`,
       filename: '/components/foo.js',
     },
+    {
+      // Assignment of tracked property with string literal property name.
+      code: `
+      import { computed } from '@ember/object';
+      import Component from '@ember/component';
+      import { tracked } from '@glimmer/tracking';
+      class MyClass extends Component {
+        @tracked 'x'
+        @computed('x') get prop() {}
+        myFunction() { this.x = 123; }
+      }`,
+      filename: '/components/foo.js',
+    },
     {
       // Assignment of tracked property (with aliased `tracked` import).
       code: `

diff --git a/tests/lib/rules/no-controllers.js b/tests/lib/rules/no-controllers.js
@@ -28,6 +28,13 @@ ruleTester.run('no-controllers', rule, {
         queryParams: ['query', 'sortType', 'sortOrder']
       });
     `,
+    // Classic class with queryParams with string literal property name.
+    `
+      import Controller from '@ember/controller';
+      export default Controller.extend({
+        'queryParams': ['query', 'sortType', 'sortOrder']
+      });
+    `,
     // Classic class with queryParams: checks object argument from variable.
     `
       import Controller from '@ember/controller';
@@ -48,6 +55,13 @@ ruleTester.run('no-controllers', rule, {
         queryParams = ['category'];
       }
     `,
+    // Native class with queryParams with string literal property name.
+    `
+      import Controller from '@ember/controller';
+      export default class ArticlesController extends Controller {
+        'queryParams' = ['category'];
+      }
+    `,
   ],
 
   invalid: [

diff --git a/tests/lib/rules/no-restricted-service-injections.js b/tests/lib/rules/no-restricted-service-injections.js
@@ -80,6 +80,14 @@ ruleTester.run('no-restricted-service-injections', rule, {
       filename: 'app/components/path.js',
       errors: [{ message: DEFAULT_ERROR_MESSAGE, type: 'Property' }],
     },
+    {
+      // Without service name argument (property name as string literal):
+      code: `${SERVICE_IMPORT} Component.extend({ 'myService': service() })`,
+      options: [{ paths: ['app/components'], services: ['my-service'] }],
+      output: null,
+      filename: 'app/components/path.js',
+      errors: [{ message: DEFAULT_ERROR_MESSAGE, type: 'Property' }],
+    },
     {
       // With camelized service name argument:
       code: `${SERVICE_IMPORT} Component.extend({ randomName: service('myService') })`,
@@ -144,6 +152,14 @@ ruleTester.run('no-restricted-service-injections', rule, {
       filename: 'app/components/path.js',
       errors: [{ message: DEFAULT_ERROR_MESSAGE, type: 'ClassProperty' }],
     },
+    {
+      // With decorator without service name argument (with parentheses) (with property name as string literal):
+      code: `${SERVICE_IMPORT} class MyComponent extends Component { @service() 'myService' }`,
+      options: [{ paths: ['app/components'], services: ['my-service'] }],
+      output: null,
+      filename: 'app/components/path.js',
+      errors: [{ message: DEFAULT_ERROR_MESSAGE, type: 'ClassProperty' }],
+    },
     {
       // With custom error message:
       code: `${SERVICE_IMPORT} Component.extend({ myService: service() })`,

diff --git a/tests/lib/rules/no-unnecessary-service-injection-argument.js b/tests/lib/rules/no-unnecessary-service-injection-argument.js
@@ -39,44 +39,22 @@ ruleTester.run('no-unnecessary-service-injection-argument', rule, {
         ecmaFeatures: { legacyDecorators: true },
       },
     },
-    {
-      code: `${RENAMED_SERVICE_IMPORT} class Test { @service() serviceName }`,
-      parser: require.resolve('@babel/eslint-parser'),
-      parserOptions: {
-        ecmaVersion: 6,
-        sourceType: 'module',
-        ecmaFeatures: { legacyDecorators: true },
-      },
-    },
+    `${RENAMED_SERVICE_IMPORT} class Test { @service() serviceName }`,
 
     // Property name matches service name but service name uses dashes
     // (allowed because it avoids needless runtime camelization <-> dasherization in the resolver):
     `${EMBER_IMPORT} export default Component.extend({ specialName: Ember.inject.service('service-name') });`,
     `${RENAMED_SERVICE_IMPORT} export default Component.extend({ specialName: service('service-name') });`,
     `${SERVICE_IMPORT} export default Component.extend({ specialName: inject('service-name') });`,
+    `${SERVICE_IMPORT} export default Component.extend({ 'specialName': inject('service-name') });`,
     `${RENAMED_SERVICE_IMPORT} const controller = Controller.extend({ serviceName: service('service-name') });`,
-    {
-      code: `${RENAMED_SERVICE_IMPORT} class Test { @service("service-name") serviceName }`,
-      parser: require.resolve('@babel/eslint-parser'),
-      parserOptions: {
-        ecmaVersion: 6,
-        sourceType: 'module',
-        ecmaFeatures: { legacyDecorators: true },
-      },
-    },
+    `${RENAMED_SERVICE_IMPORT} class Test { @service("service-name") serviceName }`,
+    `${RENAMED_SERVICE_IMPORT} class Test { @service("service-name") 'serviceName' }`,
 
     // Property name does not match service name:
     `${EMBER_IMPORT} const controller = Controller.extend({ specialName: Ember.inject.service('service-name') });`,
     `${RENAMED_SERVICE_IMPORT} const controller = Controller.extend({ specialName: service('service-name') });`,
-    {
-      code: `${RENAMED_SERVICE_IMPORT} class Test { @service("specialName") serviceName }`,
-      parser: require.resolve('@babel/eslint-parser'),
-      parserOptions: {
-        ecmaVersion: 6,
-        sourceType: 'module',
-        ecmaFeatures: { legacyDecorators: true },
-      },
-    },
+    `${RENAMED_SERVICE_IMPORT} class Test { @service("specialName") serviceName }`,
 
     // When usage is ignored because of additional arguments:
     `${EMBER_IMPORT} export default Component.extend({ serviceName: Ember.inject.service('serviceName', EXTRA_PROPERTY) });`,
@@ -86,29 +64,13 @@ ruleTester.run('no-unnecessary-service-injection-argument', rule, {
     // When usage is ignored because of template literal:
     `${EMBER_IMPORT} export default Component.extend({ serviceName: Ember.inject.service(\`serviceName\`) });`,
     `${SERVICE_IMPORT} export default Component.extend({ serviceName: service(\`serviceName\`) });`,
-    {
-      code: `${RENAMED_SERVICE_IMPORT} class Test { @service(\`specialName\`) serviceName }`,
-      parser: require.resolve('@babel/eslint-parser'),
-      parserOptions: {
-        ecmaVersion: 6,
-        sourceType: 'module',
-        ecmaFeatures: { legacyDecorators: true },
-      },
-    },
+    `${RENAMED_SERVICE_IMPORT} class Test { @service(\`specialName\`) serviceName }`,
 
     // Not Ember's `service()` function:
     "export default Component.extend({ serviceName: otherFunction('serviceName') });",
     `${RENAMED_SERVICE_IMPORT} export default Component.extend({ serviceName: service.otherFunction('serviceName') });`,
     "export default Component.extend({ serviceName: inject.otherFunction('serviceName') });",
-    {
-      code: 'class Test { @otherDecorator("name") name }',
-      parser: require.resolve('@babel/eslint-parser'),
-      parserOptions: {
-        ecmaVersion: 6,
-        sourceType: 'module',
-        ecmaFeatures: { legacyDecorators: true },
-      },
-    },
+    'class Test { @otherDecorator("name") name }',
 
     'export default Component.extend({ ...foo });',
   ],
@@ -142,12 +104,6 @@ ruleTester.run('no-unnecessary-service-injection-argument', rule, {
       code: `${RENAMED_SERVICE_IMPORT} class Test { @service("serviceName") serviceName }`,
       output: `${RENAMED_SERVICE_IMPORT} class Test { @service() serviceName }`,
       errors: [{ message: ERROR_MESSAGE, type: 'Literal' }],
-      parser: require.resolve('@babel/eslint-parser'),
-      parserOptions: {
-        ecmaVersion: 6,
-        sourceType: 'module',
-        ecmaFeatures: { legacyDecorators: true },
-      },
     },
   ],
 });
diff --git a/tests/lib/rules/no-unused-services.js b/tests/lib/rules/no-unused-services.js
@@ -139,8 +139,10 @@ function generateValid() {
     valid.push(
       `${SERVICE_IMPORT} class MyClass { @service('foo') ${SERVICE_NAME}; fooFunc() {${use}} }`,
       `${SERVICE_IMPORT} class MyClass { @service() ${SERVICE_NAME}; fooFunc() {${use}} }`,
+      `${SERVICE_IMPORT} class MyClass { @service() '${SERVICE_NAME}'; fooFunc() {${use}} }`,
       `${SERVICE_IMPORT} Component.extend({ ${SERVICE_NAME}: service('foo'), fooFunc() {${use}} });`,
-      `${SERVICE_IMPORT} Component.extend({ ${SERVICE_NAME}: service(), fooFunc() {${use}} });`
+      `${SERVICE_IMPORT} Component.extend({ ${SERVICE_NAME}: service(), fooFunc() {${use}} });`,
+      `${SERVICE_IMPORT} Component.extend({ '${SERVICE_NAME}': service(), fooFunc() {${use}} });`
     );
   }
 

diff --git a/tests/lib/rules/require-computed-property-dependencies.js b/tests/lib/rules/require-computed-property-dependencies.js
@@ -85,8 +85,10 @@ ruleTester.run('require-computed-property-dependencies', rule, {
           console.log(this.intl);
           return this.name + this.intl.t('some.translation.key');
           console.log(this.otherService);
+          console.log(this.serviceNameInStringLiteral);
         }),
-        otherService: service() // Service injection coming after computed property.
+        otherService: service(), // Service injection coming after computed property.
+        'serviceNameInStringLiteral': service() // Property name as string literal.
       });
     `,
     // Should ignore the left side of an assignment.
@@ -117,8 +119,13 @@ ruleTester.run('require-computed-property-dependencies', rule, {
       import { inject as service } from '@ember/service';
       class Test {
         @service i18n; // Service names not required as dependent keys by default.
+        @service 'serviceNameInStringLiteral';
+
         @computed('first', 'last')
         get fullName() { return this.i18n.t(this.first + ' ' + this.last); }
+
+        @computed()
+        get foo() { return this.serviceNameInStringLiteral; }
       }
     `,
   ],