From dc2b12efbd5bfc97cdcf9e472c5e2cf1f1025a45 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Sun, 30 May 2021 12:06:13 -0400 Subject: [PATCH] fix: check for string literals instead of just identifier property names in many rules --- ...ed-properties-used-in-tracking-contexts.js | 4 +- lib/rules/no-controllers.js | 8 ++- lib/rules/no-restricted-service-injections.js | 4 +- ...-unnecessary-service-injection-argument.js | 4 +- lib/rules/no-unused-services.js | 18 ++++-- .../require-computed-property-dependencies.js | 9 ++- ...ed-properties-used-in-tracking-contexts.js | 13 +++++ tests/lib/rules/no-controllers.js | 14 +++++ .../rules/no-restricted-service-injections.js | 16 +++++ ...-unnecessary-service-injection-argument.js | 58 +++---------------- tests/lib/rules/no-unused-services.js | 4 +- .../require-computed-property-dependencies.js | 9 ++- 12 files changed, 92 insertions(+), 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 index a087ed433a..bb26228d5b 100644 --- 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 index e82b65f846..c9b36cbecb 100644 --- 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 index 811dd0293e..f3689e2aa3 100644 --- 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 index 2748a6592b..73e381dd4b 100644 --- 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 index 73135c701f..3b18e0d471 100644 --- 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 index c7b20bc70f..99b489544e 100644 --- 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 index 826d0b259d..e4b1152932 100644 --- 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 index b5461b359f..a440bea749 100644 --- 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 index 4bf29bce1c..90fbef6caa 100644 --- 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 index 86adf7ae8c..da24f0c39d 100644 --- 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 index f1a4e1f25d..729624cf74 100644 --- 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 index 43c2774393..5fd8ff9827 100644 --- 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; } } `, ],