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 all 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
116 changes: 116 additions & 0 deletions docs/rules/no-implicit-injections.md
@@ -0,0 +1,116 @@
# 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 -- `denyList` -- Array of configuration objects configuring the lint rule to check for use of implicit injected services
- string -- `service` -- The (kebab-case) service name that should be checked for implicit injections and error if found
- array -- `propertyName` -- The property name where the service would be injected in classes. This defaults to the camel case version of the `service` config 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 Ember Module class files/types)

Example config:

```js
module.exports = {
rules: {
'ember/no-implicit-injections': ['error', {
denyList: [
// Ember Responsive Used to Auto Inject the media service in Components/Controllers
{ service: 'media', moduleNames: ['Component', 'Controller'] },
bmish marked this conversation as resolved.
Show resolved Hide resolved
// Ember CLI Flash Used to Auto Inject the flashMessages service in all modules
{ service: 'flash-messages' },
// Check for uses of the store in Routes or Controllers
{ service: 'store', moduleNames: ['Route', 'Controller'] },
// Check for the feature service injected as "featureChecker"
{ service: 'feature', propertyName: 'featureChecker' },
]
}]
}
}
```

## 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)
242 changes: 242 additions & 0 deletions lib/rules/no-implicit-injections.js
@@ -0,0 +1,242 @@
'use strict';

const assert = require('assert');
const emberUtils = require('../utils/ember');
const { getImportIdentifier } = require('../utils/import');
const Stack = require('../utils/stack');
const types = require('../utils/types');
const camelCase = require('lodash.camelcase');

const defaultServiceConfig = { service: 'store', moduleNames: ['Route', 'Controller'] };
const MODULE_TYPES = [
'Component',
'GlimmerComponent',
'Controller',
'Mixin',
'Route',
'Service',
'ArrayProxy',
'ObjectProxy',
'EmberObject',
'Helper',
];

// ----- -------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

function fixService(fixer, currentClass, serviceInjectImportName, failedConfig) {
const serviceInjectPath = failedConfig.service;

return currentClass.node.type === 'CallExpression'
? fixer.insertTextBefore(
currentClass.node.arguments[0].properties[0],
`${failedConfig.propertyName}: ${serviceInjectImportName}('${serviceInjectPath}'),\n`
)
: fixer.insertTextBefore(
currentClass.node.body.body[0],
`@${serviceInjectImportName}('${serviceInjectPath}') ${failedConfig.propertyName};\n`
);
}

function normalizeConfiguration(denyList) {
return denyList.map((config) => ({
service: config.service,
propertyName: config.propertyName ?? camelCase(config.service),
moduleNames: config.moduleNames ?? MODULE_TYPES,
}));
}

/** @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',
required: ['denyList'],
properties: {
denyList: {
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 use required for the denyList property too?

minItems: 1,
type: 'array',
rtablada marked this conversation as resolved.
Show resolved Hide resolved
items: {
type: 'object',
default: [defaultServiceConfig],
required: ['service'],
properties: {
rtablada marked this conversation as resolved.
Show resolved Hide resolved
service: {
type: 'string',
minLength: 1,
},
propertyName: {
type: 'string',
rtablada marked this conversation as resolved.
Show resolved Hide resolved
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
items: {
enum: MODULE_TYPES,
},
},
},
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] || {
denyList: [defaultServiceConfig],
};
const denyList = options.denyList || [defaultServiceConfig];

for (const config of denyList) {
assert(
emberUtils.convertServiceNameToKebabCase(config.service) === config.service,
'Service name should be passed in kebab-case (all lower case)'
);

assert(
!config.service.includes('/') || config.propertyName,
'Nested services must declare a property name'
);
}

// State being tracked for this file.
let serviceInjectImportName = undefined;
const normalizedConfiguration = normalizeConfiguration(denyList);

// State being tracked for the current class we're inside.
const classStack = new Stack();

// Array of posible types that could declare existing properties on native or legacy modules
const propertyDefintionTypes = new Set([
'Property',
'ClassProperty',
'PropertyDefinition',
'MethodDefinition',
]);

function onModuleFound(node) {
// Get the name of services for the current module type
let configToCheckFor = normalizedConfiguration.filter((serviceConfig) => {
return (
serviceConfig.moduleNames === undefined ||
serviceConfig.moduleNames.some((moduleName) =>
emberUtils.isEmberCoreModule(context, node, moduleName)
)
);
});

const modulePropertyDeclarations =
node.type === 'CallExpression' ? node.arguments[0].properties : node.body.body;

// Get Services that don't have properties/service injections declared
configToCheckFor = modulePropertyDeclarations.reduce((accum, n) => {
if (propertyDefintionTypes.has(n.type)) {
return accum.filter((config) => !(config.propertyName === n.key.name));
}
return accum;
}, configToCheckFor);

classStack.push({
node,
isEmberModule: true,
configToCheckFor,
});
}

function onClassEnter(node) {
if (emberUtils.isAnyEmberCoreModule(context, node)) {
onModuleFound(node);
} 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,
CallExpression(node) {
if (emberUtils.isAnyEmberCoreModule(context, node)) {
onModuleFound(node);
}
},

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

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

if (!currentClass || !currentClass.isEmberModule) {
return;
}

if (types.isThisExpression(node.object) && types.isIdentifier(node.property)) {
const failedConfig = currentClass.configToCheckFor.find(
(s) => s.propertyName === node.property.name
);

if (failedConfig) {
context.report({
node,
messageId: 'main',
data: {
serviceName: failedConfig.service,
},
fix(fixer) {
const sourceCode = context.getSourceCode();

// service inject is already declared
if (serviceInjectImportName) {
return fixService(fixer, currentClass, serviceInjectImportName, failedConfig);
}

return [
fixer.insertTextBefore(
sourceCode.ast,
"import { inject as service } from '@ember/service';\n"
),
fixService(fixer, currentClass, 'service', failedConfig),
];
},
});
}
}
},
};
},
};
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) || node.type === 'ClassExpression') {
// 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
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