Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new rule no-implicit-injections #1714

Merged
merged 22 commits into from Dec 30, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
6a50078
Implement no-implicit-injections lint rule
rtablada Dec 21, 2022
ad910a9
test: add tests for existing import for inject
rtablada Dec 21, 2022
1e98255
fix: remove no-implicit-injections from reccomended
rtablada Dec 22, 2022
86568f8
fix: use messageId for more informative lint errors
rtablada Dec 27, 2022
62d9876
test: add multi class tests
rtablada Dec 27, 2022
e8baa1d
fix: use relative md paths
rtablada Dec 27, 2022
238c372
fix: lint errors
rtablada Dec 28, 2022
595c5e5
fix: pr review feedback on documentation and config
rtablada Dec 29, 2022
c1a1cb1
fix: remove ignore when in test file
rtablada Dec 29, 2022
62ac942
fix: check validity in nested modules and classes
rtablada Dec 29, 2022
2b28afe
fix: config schema changes
rtablada Dec 29, 2022
c81f3f8
doc: add link to ember data store service
rtablada Dec 29, 2022
dadc34d
fix: config required properties and kebab case setting
rtablada Dec 30, 2022
30aea4e
fix: remove helper for isClassExpression and add test for new use
rtablada Dec 30, 2022
0ea5d4e
doc: update doc to clarify that service name should be kebab case
rtablada Dec 30, 2022
5bf714f
Update docs/rules/no-implicit-injections.md
rtablada Dec 30, 2022
05434d8
require denylist
rtablada Dec 30, 2022
89d5940
fix: check for property getters or existing definitions
rtablada Dec 30, 2022
a66d1ea
wip: testing legacy classes
rtablada Dec 30, 2022
919ce52
fix: make it work in legacy extends modules
rtablada Dec 30, 2022
61ebd4f
feat: allow custom mapping for property name
rtablada Dec 30, 2022
23b0e1a
fix: check for nested services and nested service config
rtablada Dec 30, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Expand Up @@ -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 | ✅ | | |
Expand Down
113 changes: 113 additions & 0 deletions docs/rules/no-implicit-injections.md
@@ -0,0 +1,113 @@
# ember/no-implicit-injections

🔧 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix).

<!-- end auto-generated rule header -->

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 {
model() {
return this.store.findAll('rental');
}
}

```

```js
// controllers/index.js
import Controller from '@ember/controller';
import { action } from '@ember/object';

export default class IndexController extends Controller {
@action
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;

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
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
bmish marked this conversation as resolved.
Show resolved Hide resolved
- string -- `serviceName` -- The name of the service that is implicitly injected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be consistent with the requirements for passing service names to ember/no-restricted-service-injections? All our rules should accept service names in the same format and throw an error if the service name is not in the proper format.

assert(
service.toLowerCase() === service,
'Service name should be passed in kebab-case (all lower case)'

Based on the input, that rule checks for:

  • @service myService
  • @service() myService
  • @service('myService') propertyName
  • myService: service()
  • propertyName: service('myService')
  • propertyName: service('my-service')
  • And also logic and tests for handling nested service names

Ideally, we could share code with that rule like the isInjectedServiceProp() helper for detecting the service.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this has been addressed yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/ember-cli/eslint-plugin-ember/pull/1714/files/#diff-ce4c727cc50236f1ac7ea5af2f03053469b2bff730a010ed6f0c25d29ab97319R90-R93

I added the check for rule behavior as requested and updated the md to reference that service names should be kebab cased. https://github.com/ember-cli/eslint-plugin-ember/pull/1714/files/#diff-ce4c727cc50236f1ac7ea5af2f03053469b2bff730a010ed6f0c25d29ab97319R90-R93

I'm not sure I follow what you're saying in the bullet list and I'm unclear on isInjectedServiceProp comment. I based things off the work in no-assigment-of-untracked and the patterns there since I found it to most closely resemble the state checks required to look for uses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The part about the input format for service names has been addressed.

But I still want to make sure every format of service name mentioned above is able to be detected by your rule when it checks if a service injection already exists. Can you add a test case checking for all possible formats of service injections (including nested service names) to ensure the rule is able to detect each one? You could just pass in ~10 service names to the denylist for one test case and then inject each one in a different format to ensure it's detected.

I was just suggesting isInjectedServiceProp since it could be helpful in regards to checking for service injections and handling the different formats, but not required if you already have a good way to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The service definition types aren't allowed in the deny list. The deny list config has to be the service module path.

I did however add tests for ways service could be declared in a class and added/fixed use in legacy .extends modules

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though...

Now I'm realizing that service alone isn't really 100% coverage for uses of auto-injected services.

The current config and code assumes a 1:1 mapping (👋🏽 around kebab and camelCasing) of the injected property name to the service name. But... This may not be the case: initializers can inject any property name they want:

// Example of 1:1 mapping
application.inject('controller', 'flashMessages', 'service:flash-messages');

// Example of arbitrary mapping
application.inject('controller', 'somethingElse', 'service:flash-messages');

I think the config feels good for the 80% usecase (and TBH most teams will likely not configure at all and just look for implicit use of store.

We could augment this with a config propertyName which would default to camelCase(config.service)

Copy link
Member

@bmish bmish Dec 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we care about that edge case at all (maybe not?), we can worry about it as a follow-up. I'm ready to merge this PR as soon as I am sure nested services are handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thought of propertyName came from the issue with nested services`. Ember AFAIK doesn't have a way of auto mapping a property to a nested service.

So if you have an autoInjection of @service('cart/checkout') there has to be an explicit propertyName. I'll add an assertion to check for this config.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I would still like to see a test with the nested service name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated and added the assertion mentioned 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 class files/types)
bmish marked this conversation as resolved.
Show resolved Hide resolved
rtablada marked this conversation as resolved.
Show resolved Hide resolved

Example config:

```js
module.exports = {
rules: {
'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'] },
]
}]
}
}
```

## References
rtablada marked this conversation as resolved.
Show resolved Hide resolved

- [Deprecation](https://deprecations.emberjs.com/v3.x/#toc_implicit-injections)
- [Ember Data Store Service](https://api.emberjs.com/ember-data/release/classes/Store)
198 changes: 198 additions & 0 deletions lib/rules/no-implicit-injections.js
@@ -0,0 +1,198 @@
'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'] };

//------------------------------------------------------------------------------
// 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: false,
url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-implicit-injections.md',
},
fixable: 'code',
schema: [
{
type: 'object',
properties: {
services: {
minItems: 1,
type: 'array',
rtablada marked this conversation as resolved.
Show resolved Hide resolved
items: {
type: 'object',
default: [defaultServiceConfig],
properties: {
rtablada marked this conversation as resolved.
Show resolved Hide resolved
serviceName: {
type: 'string',
rtablada marked this conversation as resolved.
Show resolved Hide resolved
required: true,
minLength: 1,
},
moduleNames: {
rtablada marked this conversation as resolved.
Show resolved Hide resolved
type: 'array',
rtablada marked this conversation as resolved.
Show resolved Hide resolved
required: false,
items: {
enum: [
'Component',
'GlimmerComponent',
'Controller',
'Mixin',
'Route',
'Service',
'ArrayProxy',
'ObjectProxy',
'EmberObject',
'Helper',
],
},
},
},
additionalProperties: false,
},
},
},
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.',
},
},

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;

// 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') {
serviceInjectImportName =
serviceInjectImportName || getImportIdentifier(node, '@ember/service', 'inject');
}
},

ClassDeclaration: onClassEnter,
ClassExpression: onClassEnter,

'ClassDeclaration:exit': onClassExit,
'ClassExpression:exit': onClassExit,

MemberExpression(node) {
const currentClass = classStack.peek();

if (!currentClass || !currentClass.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,
messageId: 'main',
data: {
serviceName: serviceMissingInjection,
},
fix(fixer) {
const sourceCode = context.getSourceCode();

// 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),
];
},
});
}
}
},
};
},
};
4 changes: 2 additions & 2 deletions lib/utils/ember.js
Expand Up @@ -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)) {
bmish marked this conversation as resolved.
Show resolved Hide resolved
rtablada marked this conversation as resolved.
Show resolved Hide resolved
// native classes
if (
// class Foo extends Component
Expand Down Expand Up @@ -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;
Expand Down
11 changes: 11 additions & 0 deletions lib/utils/types.js
Expand Up @@ -9,6 +9,7 @@ module.exports = {
isCallExpression,
isCallWithFunctionExpression,
isClassDeclaration,
isClassExpression,
isClassPropertyOrPropertyDefinition,
isCommaToken,
isConciseArrowFunctionWithCallExpression,
Expand Down Expand Up @@ -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) {
rtablada marked this conversation as resolved.
Show resolved Hide resolved
return node !== undefined && node.type === 'ClassExpression';
}

/**
* Check whether or not a node is a ClassProperty (ESLint v7) or PropertyDefinition (ESLint v8).
*
Expand Down
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -71,6 +71,7 @@
"ember-template-imports": "^3.1.1",
"eslint-utils": "^3.0.0",
"estraverse": "^5.2.0",
"lodash.camelcase": "^4.1.1",
"lodash.kebabcase": "^4.1.1",
"magic-string": "^0.27.0",
"requireindex": "^1.2.0",
Expand Down