From 049cf07851c4590b71afdd24d8fafeeef33f18c4 Mon Sep 17 00:00:00 2001 From: Peter Wagenet Date: Tue, 3 Jan 2023 10:50:02 -0800 Subject: [PATCH 1/2] Add tests for new service import --- ...no-deprecated-router-transition-methods.js | 37 +++++++++++++++++++ .../no-implicit-service-injection-argument.js | 9 ++++- tests/lib/rules/no-private-routing-service.js | 24 ++++++++++++ .../rules/no-restricted-service-injections.js | 15 ++++++++ ...-unnecessary-service-injection-argument.js | 8 ++++ tests/lib/rules/no-unused-services.js | 37 ++++++++++++++----- .../require-computed-property-dependencies.js | 17 +++++++++ 7 files changed, 137 insertions(+), 10 deletions(-) diff --git a/tests/lib/rules/no-deprecated-router-transition-methods.js b/tests/lib/rules/no-deprecated-router-transition-methods.js index 2ac605f7e8..1752e96621 100644 --- a/tests/lib/rules/no-deprecated-router-transition-methods.js +++ b/tests/lib/rules/no-deprecated-router-transition-methods.js @@ -680,6 +680,43 @@ import Controller from '@ember/controller'; }, ], }, + // Injecting with `service` export + { + filename: 'routes/index.js', + code: ` + import Route from '@ember/routing/route'; + import { service } from '@ember/service'; + + export default class SettingsRoute extends Route { + @service session; + + beforeModel() { + if (!this.session.isAuthenticated) { + this.transitionTo('login'); + } + } + }`, + output: ` + import Route from '@ember/routing/route'; + import { service } from '@ember/service'; + + export default class SettingsRoute extends Route { + @service('router') router; +@service session; + + beforeModel() { + if (!this.session.isAuthenticated) { + this.router.transitionTo('login'); + } + } + }`, + errors: [ + { + messageId: 'main', + type: 'MemberExpression', + }, + ], + }, // Test multiple classes in a single file { filename: 'routes/index.js', diff --git a/tests/lib/rules/no-implicit-service-injection-argument.js b/tests/lib/rules/no-implicit-service-injection-argument.js index 8acc3f1c6a..28a146eb57 100644 --- a/tests/lib/rules/no-implicit-service-injection-argument.js +++ b/tests/lib/rules/no-implicit-service-injection-argument.js @@ -10,6 +10,7 @@ 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';"; +const NEW_SERVICE_IMPORT = "import {service} from '@ember/service';"; //------------------------------------------------------------------------------ // Tests @@ -36,6 +37,7 @@ ruleTester.run('no-implicit-service-injection-argument', rule, { // With argument (native class) `${SERVICE_IMPORT} class Test { @service('service-name') serviceName }`, + `${NEW_SERVICE_IMPORT} class Test { @service('service-name') serviceName }`, // Not Ember's `service()` function (classic class): 'export default Component.extend({ serviceName: otherFunction() });', @@ -69,13 +71,18 @@ ruleTester.run('no-implicit-service-injection-argument', rule, { 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 new import + { + code: `${NEW_SERVICE_IMPORT} class Test { @service() serviceName }`, + output: `${NEW_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 }`, diff --git a/tests/lib/rules/no-private-routing-service.js b/tests/lib/rules/no-private-routing-service.js index 1e668e207a..e95581fe50 100644 --- a/tests/lib/rules/no-private-routing-service.js +++ b/tests/lib/rules/no-private-routing-service.js @@ -13,6 +13,7 @@ const { const EMBER_IMPORT = "import Ember from 'ember';"; const SERVICE_IMPORT = "import {inject as service} from '@ember/service';"; +const NEW_SERVICE_IMPORT = "import {service} from '@ember/service';"; //------------------------------------------------------------------------------ // Tests @@ -47,6 +48,7 @@ ruleTester.run('no-private-routing-service', rule, { `${SERVICE_IMPORT} export default class MyComponent extends Component { @service('router') routing; }`, `${SERVICE_IMPORT} export default class MyComponent extends Component { @service routing; }`, `${SERVICE_IMPORT} export default class MyComponent extends Component { @service('routing') routing; }`, + `${NEW_SERVICE_IMPORT} export default class MyComponent extends Component { @service('routing') routing; }`, ` export default class MyComponent extends Component { @computed('-routing', 'lastName') @@ -95,6 +97,28 @@ ruleTester.run('no-private-routing-service', rule, { }, ], }, + // Octane, new import + { + code: `${NEW_SERVICE_IMPORT} export default class MyComponent extends Component { @service('-routing') routing; }`, + output: null, + errors: [ + { + message: PRIVATE_ROUTING_SERVICE_ERROR_MESSAGE, + // type could be ClassProperty (ESLint v7) or PropertyDefinition (ESLint v8) + }, + ], + }, + // Octane, new import, renamed + { + code: "import {service as aliasedService} from '@ember/service'; export default class MyComponent extends Component { @aliasedService('-routing') routing; }", + output: null, + errors: [ + { + message: PRIVATE_ROUTING_SERVICE_ERROR_MESSAGE, + // type could be ClassProperty (ESLint v7) or PropertyDefinition (ESLint v8) + }, + ], + }, // _routerMicrolib (`catchRouterMicrolib` option on) { diff --git a/tests/lib/rules/no-restricted-service-injections.js b/tests/lib/rules/no-restricted-service-injections.js index a5de958551..d3eea96762 100644 --- a/tests/lib/rules/no-restricted-service-injections.js +++ b/tests/lib/rules/no-restricted-service-injections.js @@ -5,6 +5,7 @@ const { DEFAULT_ERROR_MESSAGE } = rule; const EMBER_IMPORT = "import Ember from 'ember';"; const SERVICE_IMPORT = "import {inject as service} from '@ember/service';"; +const NEW_SERVICE_IMPORT = "import {service} from '@ember/service';"; const ruleTester = new RuleTester({ parser: require.resolve('@babel/eslint-parser'), @@ -46,6 +47,12 @@ ruleTester.run('no-restricted-service-injections', rule, { code: `${SERVICE_IMPORT} class MyComponent extends Component { @service('myService') randomName }`, options: [{ paths: ['app/components'], services: ['abc'] }], }, + { + // Service name doesn't match (with decorator and new import) + filename: 'app/components/path.js', + code: `${NEW_SERVICE_IMPORT} class MyComponent extends Component { @service('myService') randomName }`, + options: [{ paths: ['app/components'], services: ['abc'] }], + }, { // Service scope doesn't match: filename: 'app/components/path.js', @@ -80,6 +87,14 @@ ruleTester.run('no-restricted-service-injections', rule, { options: [{ paths: ['app/components'], services: ['my-service'] }], errors: [{ message: DEFAULT_ERROR_MESSAGE, type: 'Property' }], }, + { + // Without service name argument and new import: + filename: 'app/components/path.js', + code: `${NEW_SERVICE_IMPORT} Component.extend({ myService: service() })`, + output: null, + options: [{ paths: ['app/components'], services: ['my-service'] }], + errors: [{ message: DEFAULT_ERROR_MESSAGE, type: 'Property' }], + }, { // Without service name argument (property name as string literal): filename: 'app/components/path.js', diff --git a/tests/lib/rules/no-unnecessary-service-injection-argument.js b/tests/lib/rules/no-unnecessary-service-injection-argument.js index 55d04913df..79822b4ceb 100644 --- a/tests/lib/rules/no-unnecessary-service-injection-argument.js +++ b/tests/lib/rules/no-unnecessary-service-injection-argument.js @@ -10,6 +10,7 @@ const { ERROR_MESSAGE } = rule; const EMBER_IMPORT = "import Ember from 'ember';"; const SERVICE_IMPORT = "import {inject} from '@ember/service';"; const RENAMED_SERVICE_IMPORT = "import {inject as service} from '@ember/service';"; +const NEW_SERVICE_IMPORT = "import {service} from '@ember/service';"; //------------------------------------------------------------------------------ // Tests @@ -50,6 +51,7 @@ ruleTester.run('no-unnecessary-service-injection-argument', rule, { `${RENAMED_SERVICE_IMPORT} const controller = Controller.extend({ serviceName: service('service-name') });`, `${RENAMED_SERVICE_IMPORT} class Test { @service("service-name") serviceName }`, `${RENAMED_SERVICE_IMPORT} class Test { @service("service-name") 'serviceName' }`, + `${NEW_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') });`, @@ -105,5 +107,11 @@ ruleTester.run('no-unnecessary-service-injection-argument', rule, { output: `${RENAMED_SERVICE_IMPORT} class Test { @service() serviceName }`, errors: [{ message: ERROR_MESSAGE, type: 'Literal' }], }, + // Decorator, with new import + { + code: `${NEW_SERVICE_IMPORT} class Test { @service("serviceName") serviceName }`, + output: `${NEW_SERVICE_IMPORT} class Test { @service() serviceName }`, + errors: [{ message: ERROR_MESSAGE, type: 'Literal' }], + }, ], }); diff --git a/tests/lib/rules/no-unused-services.js b/tests/lib/rules/no-unused-services.js index 6e35988d4c..3335f1380d 100644 --- a/tests/lib/rules/no-unused-services.js +++ b/tests/lib/rules/no-unused-services.js @@ -19,6 +19,7 @@ const ruleTester = new RuleTester({ const SERVICE_NAME = 'fooName'; const SERVICE_IMPORT = "import {inject as service} from '@ember/service';"; +const NEW_SERVICE_IMPORT = "import {service} from '@ember/service';"; const EO_IMPORTS = "import {computed, get, getProperties, observer} from '@ember/object';"; const RENAMED_EO_IMPORTS = "import {computed as cp, get as g, getProperties as gp, observer as ob} from '@ember/object';"; @@ -131,18 +132,18 @@ function generateObserverUseCasesFor(propertyName, renamed = false) { * Generate an array of valid test cases * @returns {Array} */ -function generateValid() { +function generateValid(importString = SERVICE_IMPORT) { const valid = []; const useCases = generateUseCasesFor(SERVICE_NAME); for (const use of useCases) { 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}} });` + `${importString} class MyClass { @service('foo') ${SERVICE_NAME}; fooFunc() {${use}} }`, + `${importString} class MyClass { @service() ${SERVICE_NAME}; fooFunc() {${use}} }`, + `${importString} class MyClass { @service() '${SERVICE_NAME}'; fooFunc() {${use}} }`, + `${importString} Component.extend({ ${SERVICE_NAME}: service('foo'), fooFunc() {${use}} });`, + `${importString} Component.extend({ ${SERVICE_NAME}: service(), fooFunc() {${use}} });`, + `${importString} Component.extend({ '${SERVICE_NAME}': service(), fooFunc() {${use}} });` ); } @@ -154,8 +155,8 @@ function generateValid() { const imports = idx === 0 ? EO_IMPORTS : RENAMED_EO_IMPORTS; for (const use of useCases) { valid.push( - `${SERVICE_IMPORT}${imports} class MyClass { @service() ${SERVICE_NAME}; fooFunc() {${use}} }`, - `${SERVICE_IMPORT}${imports} Component.extend({ ${SERVICE_NAME}: service(), fooFunc() {${use}} });` + `${importString}${imports} class MyClass { @service() ${SERVICE_NAME}; fooFunc() {${use}} }`, + `${importString}${imports} Component.extend({ ${SERVICE_NAME}: service(), fooFunc() {${use}} });` ); } } @@ -173,6 +174,7 @@ const emberObjectUses2 = generateEmberObjectUseCasesFor('unrelatedProp').join('' ruleTester.run('no-unused-services', rule, { valid: [ ...generateValid(), + ...generateValid(NEW_SERVICE_IMPORT), ...generateMacroUseCasesFor(SERVICE_NAME), ...generateMacroUseCasesFor(SERVICE_NAME, true), ...generateComputedUseCasesFor(SERVICE_NAME), @@ -203,6 +205,23 @@ ruleTester.run('no-unused-services', rule, { }, ], }, + // With new import + { + code: `${NEW_SERVICE_IMPORT} class MyClass { @service('foo') ${SERVICE_NAME}; fooFunc() {${nonUses}} }`, + output: null, + errors: [ + { + messageId: 'main', + suggestions: [ + { + messageId: 'removeServiceInjection', + output: `${NEW_SERVICE_IMPORT} class MyClass { fooFunc() {${nonUses}} }`, + }, + ], + // type could be ClassProperty (ESLint v7) or PropertyDefinition (ESLint v8) + }, + ], + }, { code: `${SERVICE_IMPORT} class MyClass { @service() ${SERVICE_NAME}; fooFunc() {${nonUses}} }`, output: null, diff --git a/tests/lib/rules/require-computed-property-dependencies.js b/tests/lib/rules/require-computed-property-dependencies.js index 6711a84cb6..59c652f4a7 100644 --- a/tests/lib/rules/require-computed-property-dependencies.js +++ b/tests/lib/rules/require-computed-property-dependencies.js @@ -93,6 +93,23 @@ ruleTester.run('require-computed-property-dependencies', rule, { 'serviceNameInStringLiteral': service() // Property name as string literal. }); `, + // Should ignore injected service names via `service` method: + ` + import Ember from 'ember'; + import Component from '@ember/component'; + import { service } from '@ember/service'; + Component.extend({ + intl: service(), + myProperty: Ember.computed('name', function() { + 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. + 'serviceNameInStringLiteral': service() // Property name as string literal. + }); + `, // Should ignore the left side of an assignment. "import Ember from 'ember'; Ember.computed('right', function() { this.left = this.right; })", // Should ignore the left side of an assignment with nested path. From 3341a79049c925150c2d4825945061b3b92493f5 Mon Sep 17 00:00:00 2001 From: Peter Wagenet Date: Tue, 3 Jan 2023 11:09:40 -0800 Subject: [PATCH 2/2] Properly handle new service import --- .../no-deprecated-router-transition-methods.js | 5 ++++- .../no-implicit-service-injection-argument.js | 4 +++- lib/rules/no-private-routing-service.js | 16 +++++++++++----- lib/rules/no-restricted-service-injections.js | 4 +++- .../no-unnecessary-service-injection-argument.js | 4 +++- lib/rules/no-unused-services.js | 4 +++- .../require-computed-property-dependencies.js | 4 +++- 7 files changed, 30 insertions(+), 11 deletions(-) diff --git a/lib/rules/no-deprecated-router-transition-methods.js b/lib/rules/no-deprecated-router-transition-methods.js index 1b691c778e..60b5c1eb20 100644 --- a/lib/rules/no-deprecated-router-transition-methods.js +++ b/lib/rules/no-deprecated-router-transition-methods.js @@ -12,6 +12,7 @@ function getBaseFixSteps(fixer, context, currentClass) { let serviceInjectImportName = currentClass.serviceInjectImportName; let routerServicePropertyName = currentClass.routerServicePropertyName; + // For now, we insert the legacy form. If we can detect the Ember version we can insert the new version instead. if (!serviceInjectImportName) { fixSteps.push( fixer.insertTextBefore( @@ -130,7 +131,9 @@ module.exports = { ImportDeclaration(node) { if (node.source.value === '@ember/service') { serviceInjectImportName = - serviceInjectImportName || getImportIdentifier(node, '@ember/service', 'inject'); + serviceInjectImportName || + getImportIdentifier(node, '@ember/service', 'inject') || + getImportIdentifier(node, '@ember/service', 'service'); } }, diff --git a/lib/rules/no-implicit-service-injection-argument.js b/lib/rules/no-implicit-service-injection-argument.js index 075c156804..f7659e46aa 100644 --- a/lib/rules/no-implicit-service-injection-argument.js +++ b/lib/rules/no-implicit-service-injection-argument.js @@ -79,7 +79,9 @@ module.exports = { } if (node.source.value === '@ember/service') { importedInjectName = - importedInjectName || getImportIdentifier(node, '@ember/service', 'inject'); + importedInjectName || + getImportIdentifier(node, '@ember/service', 'inject') || + getImportIdentifier(node, '@ember/service', 'service'); } }, Property(node) { diff --git a/lib/rules/no-private-routing-service.js b/lib/rules/no-private-routing-service.js index 19d624d7dc..80d0a29273 100644 --- a/lib/rules/no-private-routing-service.js +++ b/lib/rules/no-private-routing-service.js @@ -62,10 +62,10 @@ module.exports = { let importedEmberName; // Handle either ClassProperty (ESLint v7) or PropertyDefinition (ESLint v8). - function visitClassPropertyOrPropertyDefinition(node) { + function visitClassPropertyOrPropertyDefinition(node, importedInjectName) { if ( !node.decorators || - !decoratorUtils.isClassPropertyOrPropertyDefinitionWithDecorator(node, 'service') + !decoratorUtils.isClassPropertyOrPropertyDefinitionWithDecorator(node, importedInjectName) ) { return; } @@ -92,7 +92,9 @@ module.exports = { } if (node.source.value === '@ember/service') { importedInjectName = - importedInjectName || getImportIdentifier(node, '@ember/service', 'inject'); + importedInjectName || + getImportIdentifier(node, '@ember/service', 'inject') || + getImportIdentifier(node, '@ember/service', 'service'); } }, Property(node) { @@ -105,8 +107,12 @@ module.exports = { } }, - ClassProperty: visitClassPropertyOrPropertyDefinition, - PropertyDefinition: visitClassPropertyOrPropertyDefinition, + ClassProperty(node) { + visitClassPropertyOrPropertyDefinition(node, importedInjectName); + }, + PropertyDefinition(node) { + visitClassPropertyOrPropertyDefinition(node, importedInjectName); + }, Literal(node) { if ( diff --git a/lib/rules/no-restricted-service-injections.js b/lib/rules/no-restricted-service-injections.js index 912037708f..9504e6636f 100644 --- a/lib/rules/no-restricted-service-injections.js +++ b/lib/rules/no-restricted-service-injections.js @@ -123,7 +123,9 @@ module.exports = { } if (node.source.value === '@ember/service') { importedInjectName = - importedInjectName || getImportIdentifier(node, '@ember/service', 'inject'); + importedInjectName || + getImportIdentifier(node, '@ember/service', 'inject') || + getImportIdentifier(node, '@ember/service', 'service'); } }, // Handles: diff --git a/lib/rules/no-unnecessary-service-injection-argument.js b/lib/rules/no-unnecessary-service-injection-argument.js index 99410a765f..340ae79616 100644 --- a/lib/rules/no-unnecessary-service-injection-argument.js +++ b/lib/rules/no-unnecessary-service-injection-argument.js @@ -64,7 +64,9 @@ module.exports = { } if (node.source.value === '@ember/service') { importedInjectName = - importedInjectName || getImportIdentifier(node, '@ember/service', 'inject'); + importedInjectName || + getImportIdentifier(node, '@ember/service', 'inject') || + getImportIdentifier(node, '@ember/service', 'service'); } }, Property(node) { diff --git a/lib/rules/no-unused-services.js b/lib/rules/no-unused-services.js index 8a0dc826de..3d577a1c0a 100644 --- a/lib/rules/no-unused-services.js +++ b/lib/rules/no-unused-services.js @@ -139,7 +139,9 @@ module.exports = { } if (node.source.value === '@ember/service') { importedInjectName = - importedInjectName || getImportIdentifier(node, '@ember/service', 'inject'); + importedInjectName || + getImportIdentifier(node, '@ember/service', 'inject') || + getImportIdentifier(node, '@ember/service', 'service'); } if (node.source.value === '@ember-decorators/object') { importedObservesName = diff --git a/lib/rules/require-computed-property-dependencies.js b/lib/rules/require-computed-property-dependencies.js index 55cb28b336..cd58b03e1d 100644 --- a/lib/rules/require-computed-property-dependencies.js +++ b/lib/rules/require-computed-property-dependencies.js @@ -449,7 +449,9 @@ module.exports = { } if (node.source.value === '@ember/service') { importedInjectName = - importedInjectName || getImportIdentifier(node, '@ember/service', 'inject'); + importedInjectName || + getImportIdentifier(node, '@ember/service', 'inject') || + getImportIdentifier(node, '@ember/service', 'service'); } },