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-get-with-default #594

Merged
merged 12 commits into from
Nov 19, 2019
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ The `--fix` option on the command line automatically fixes problems reported by
| | [named-functions-in-promises](./docs/rules/named-functions-in-promises.md) | Enforces usage of named functions in promises |
| :white_check_mark: | [new-module-imports](./docs/rules/new-module-imports.md) | Use "New Module Imports" from Ember RFC #176 |
| :white_check_mark: | [no-function-prototype-extensions](./docs/rules/no-function-prototype-extensions.md) | Prevents usage of Ember's `function` prototype extensions |
| :car: | [no-get-with-default](./docs/rules/no-get-with-default.md) | Use the `||` operator instead of `getWithDefault` for more expected behaviors |
| :car::wrench: | [no-get](./docs/rules/no-get.md) | Require ES5 getters instead of Ember's `get` / `getProperties` functions |
| :white_check_mark: | [no-global-jquery](./docs/rules/no-global-jquery.md) | Prevents usage of global jQuery object |
| :car: | [no-jquery](./docs/rules/no-jquery.md) | Disallow any usage of jQuery |
Expand Down
41 changes: 41 additions & 0 deletions docs/rules/no-get-with-default.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# No `getWithDefault` (no-get-with-default)

This rule attempts to catch and prevent the use of `getWithDefault`.

## Rule Details

Even though the behavior for `getWithDefault` is more defined such that it only falls back to the default value on `undefined`,
its inconsistency with the native `||` is confusing to many developers who assume otherwise. This rule encourages developers to use
the native `||` operator instead.
steventsao marked this conversation as resolved.
Show resolved Hide resolved

In addition, [Nullish Coalescing Operator `??`](https://github.com/tc39/proposal-nullish-coalescing) will land in the JavaScript language soon so developers can leverage safe property access with native support instead of using `getWithDefault`.

This rule **forbids** the following:

```js
const test = this.getWithDefault('key', []);
```

```js
const test = getWithDefault(this, 'key', []);
```

This rule **allows** the following:
steventsao marked this conversation as resolved.
Show resolved Hide resolved
steventsao marked this conversation as resolved.
Show resolved Hide resolved

```js
const test = this.key === undefined ? [] : this.key;
```

```js
// the behavior of this is different because `test` would be assigned `[]` on any falsy value instead of on only `undefined`.
const test = this.key || [];
```

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

- [RFC](https://github.com/emberjs/rfcs/pull/554/) to deprecate `getWithDefault`

## Related Rules

- [no-get](https://github.com/ember-cli/eslint-plugin-ember/blob/master/docs/rules/no-get.md)
- [spec](http://api.emberjs.com/ember/3.13/functions/@ember%2Fobject/getWithDefault)
steventsao marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ module.exports = {
'no-ember-testing-in-module-scope': require('./rules/no-ember-testing-in-module-scope'),
'no-empty-attrs': require('./rules/no-empty-attrs'),
'no-function-prototype-extensions': require('./rules/no-function-prototype-extensions'),
'no-get-with-default': require('./rules/no-get-with-default'),
'no-get': require('./rules/no-get'),
'no-global-jquery': require('./rules/no-global-jquery'),
'no-incorrect-calls-with-inline-anonymous-functions': require('./rules/no-incorrect-calls-with-inline-anonymous-functions'),
Expand Down
1 change: 1 addition & 0 deletions lib/octane-rules.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ module.exports = {
"ember/no-classic-classes": "error",
"ember/no-classic-components": "error",
"ember/no-computed-properties-in-native-classes": "error",
"ember/no-get-with-default": "error",
"ember/no-get": "error",
"ember/no-jquery": "error",
"ember/require-tagless-components": "error"
Expand Down
1 change: 1 addition & 0 deletions lib/recommended-rules.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ module.exports = {
"ember/no-ember-testing-in-module-scope": "error",
"ember/no-empty-attrs": "off",
"ember/no-function-prototype-extensions": "error",
"ember/no-get-with-default": "off",
"ember/no-get": "off",
"ember/no-global-jquery": "error",
"ember/no-incorrect-calls-with-inline-anonymous-functions": "error",
Expand Down
49 changes: 49 additions & 0 deletions lib/rules/no-get-with-default.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
'use strict';

const types = require('../utils/types');

function makeErrorMessageForGetWithDefault(property, isImportedGetWithDefault) {
return isImportedGetWithDefault
steventsao marked this conversation as resolved.
Show resolved Hide resolved
? 'Use `||` or the ternary operator instead of `getWithDefault()`.'
: 'Use `this.property || defaultValue` or the `this.property === undefined ? defaultValue : this.property` instead of `getWithDefault(this.property, defaultValue)`.';
}

module.exports = {
meta: {
type: 'suggestion',
docs: {
description: 'Use the `||` operator instead of `getWithDefault` for more expected behaviors',
steventsao marked this conversation as resolved.
Show resolved Hide resolved
category: 'Best Practices',
recommended: false,
octane: true,
steventsao marked this conversation as resolved.
Show resolved Hide resolved
url:
'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-get-with-default.md',
},
},
create(context) {
return {
CallExpression(node) {
if (
types.isMemberExpression(node.callee) &&
types.isThisExpression(node.callee.object) &&
types.isIdentifier(node.callee.property) &&
node.callee.property.name === 'getWithDefault' &&
node.arguments.length === 2
) {
// Example: this.getWithDefault('foo', 'bar');
context.report(node, makeErrorMessageForGetWithDefault(node.arguments[0].value, false));
}

if (
types.isIdentifier(node.callee) &&
node.callee.name === 'getWithDefault' &&
node.arguments.length === 3 &&
types.isThisExpression(node.arguments[0])
steventsao marked this conversation as resolved.
Show resolved Hide resolved
) {
// Example: getWithDefault(this, 'foo', 'bar');
context.report(node, makeErrorMessageForGetWithDefault(node.arguments[1].value, true));
}
},
};
},
};
41 changes: 41 additions & 0 deletions tests/lib/rules/no-get-with-default.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
const rule = require('../../../lib/rules/no-get-with-default');
const RuleTester = require('eslint').RuleTester;

const ruleTester = new RuleTester({
parserOptions: {
ecmaVersion: 2015,
sourceType: 'module',
},
});

ruleTester.run('no-get-with-default', rule, {
valid: [
"const test = this.get('key') || [];",
"const test = get(this, 'target') || [];",
"testClass.getWithDefault('key', [])",
"getWithDefault.testMethod(testClass, 'key', [])",
],
invalid: [
{
code: "const test = this.getWithDefault('key', []);",
errors: [
{
message:
'Use `this.property || defaultValue` or the `this.property === undefined ? defaultValue : this.property` instead of `getWithDefault(this.property, defaultValue)`.',
type: 'CallExpression',
},
],
output: null,
},
{
steventsao marked this conversation as resolved.
Show resolved Hide resolved
code: "const test = getWithDefault(this, 'key', []);",
errors: [
{
message: 'Use `||` or the ternary operator instead of `getWithDefault()`.',
type: 'CallExpression',
},
],
output: null,
},
],
});