From 468661fcd6862eb09ca99463bde84d7591190a7d Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Thu, 21 Mar 2019 22:01:07 -0700 Subject: [PATCH 1/3] Add rule to enforce consistent computed property style Defaults to `never` allow getters in computed properties, has the option to `always` require them. --- README.md | 1 + docs/rules/computed-property-getters.md | 57 +++++++++ lib/rules/computed-property-getters.js | 67 ++++++++++ tests/lib/rules/computed-property-getters.js | 126 +++++++++++++++++++ 4 files changed, 251 insertions(+) create mode 100644 docs/rules/computed-property-getters.md create mode 100644 lib/rules/computed-property-getters.js create mode 100644 tests/lib/rules/computed-property-getters.js diff --git a/README.md b/README.md index 0bc38a0154..f69c95e1dd 100644 --- a/README.md +++ b/README.md @@ -127,6 +127,7 @@ The `--fix` option on the command line automatically fixes problems reported by | | Rule ID | Description | |:---|:--------|:------------| | :white_check_mark: | [avoid-leaking-state-in-ember-objects](./docs/rules/avoid-leaking-state-in-ember-objects.md) | Avoids state leakage | +| | [computed-property-getters](./docs/rules/computed-property-getters.md) | Enforce the consistent use of getters in computed properties | ### Testing diff --git a/docs/rules/computed-property-getters.md b/docs/rules/computed-property-getters.md new file mode 100644 index 0000000000..b79fc48931 --- /dev/null +++ b/docs/rules/computed-property-getters.md @@ -0,0 +1,57 @@ +# Rule name: `computed-property-getters` + +## Enforce the consistent use of getters in computed properties + +Computed properties may be created with or without a `get` method. This rule ensures that the choice +is consistent. + +## Options + +This rule takes a single string option + +String option: + +* `"never"` (default) getters are *never allowed* in computed properties +* `"always"` getters are *required* in computed properties + +### never + +```javascript +/// GOOD +Ember.Object.extend({ + fullName: computed('firstName', 'lastName', function() { + //... + }) +}); + +// BAD +Ember.Object.extend({ + fullName: computed('firstName', 'lastName', { + get() { + //... + } + }) +}); +``` + +### always + +```javascript +/// GOOD +Ember.Object.extend({ + fullName: computed('firstName', 'lastName', { + get() { + //... + } + }) +}); + +// BAD +Ember.Object.extend({ + fullName: computed('firstName', 'lastName', function() { + //... + }) +}); +``` + + diff --git a/lib/rules/computed-property-getters.js b/lib/rules/computed-property-getters.js new file mode 100644 index 0000000000..78e0828019 --- /dev/null +++ b/lib/rules/computed-property-getters.js @@ -0,0 +1,67 @@ +'use strict'; + +const ember = require('../utils/ember'); +const utils = require('../utils/utils'); + +//------------------------------------------------------------------------------ +// General rule - Prevent using a getter inside computed properties. +//------------------------------------------------------------------------------ + +module.exports = { + meta: { + docs: { + description: 'Warns about getters in computed properties', + category: 'Possible Errors', + recommended: false, + url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/computed-property-getters.md' + }, + fixable: null, // or "code" or "whitespace" + schema: [ + { + enum: ['always', 'never'] + }, + ], + }, + + create(context) { + const requireGetters = context.options[0] || 'never'; + const message = 'Do not use a getter inside computed properties.'; + + const report = function (node) { + context.report(node, message); + }; + + const preventGetterInComputedProperty = function (node) { + const objectExpressions = node.arguments.filter(arg => utils.isObjectExpression(arg)); + if ( + objectExpressions.length + ) { + report(node); + } + }; + + const requireGetterInComputedProperty = function (node) { + const functionExpressions = node.arguments.filter(arg => utils.isFunctionExpression(arg)); + if ( + functionExpressions.length + ) { + report(node); + } + }; + + return { + CallExpression(node) { + if ( + ember.isComputedProp(node) && + node.arguments.length + ) { + if (requireGetters === 'always') { + requireGetterInComputedProperty(node); + } else { + preventGetterInComputedProperty(node); + } + } + } + }; + } +}; diff --git a/tests/lib/rules/computed-property-getters.js b/tests/lib/rules/computed-property-getters.js new file mode 100644 index 0000000000..facee06124 --- /dev/null +++ b/tests/lib/rules/computed-property-getters.js @@ -0,0 +1,126 @@ +'use strict'; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require('../../../lib/rules/computed-property-getters'); +const RuleTester = require('eslint').RuleTester; + + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const ruleTester = new RuleTester(); +const parserOptions = { ecmaVersion: 2018, sourceType: 'module' }; + +const codeWithoutGetters = [ + ` + { + foo: computed('model', function() {}) + }`, + `{ + foo: computed('model', function() {}).volatile() + }`, + `{ + foo: computed(function() {}) + }`, + `{ + foo: computed(async function() {}) + }`, + `{ + foo: computed('model', async function() {}) + }` +]; + +const codeWithGetters = [ + `{ + foo: computed({ + get() { + return true; + } + }).readonly() + }`, + `{ + foo: computed({ + get() { + return true; + } + }) + }`, + `{ + foo: computed('model.foo', { + get() { + return true; + } + }).readonly() + }`, + `{ + foo: computed('model.foo', { + get() { + return true; + } + }) + }`, + `{ + foo: computed('model.foo', { + get() {} + }) + }` +]; + + +const validWithDefaultOptions = codeWithoutGetters.map(code => ({ code, parserOptions })); + +const validWithNeverOption = codeWithoutGetters.map((code) => { + const options = ['never']; + return { code, parserOptions, options }; +}); + +const validWithAlwaysOption = codeWithGetters.map((code) => { + const options = ['always']; + return { code, parserOptions, options }; +}); + +const inValidWithDefaultOptions = codeWithGetters.map(code => ( + { + code, + parserOptions, + output: null, + errors: [{ + message: rule.meta.message, + }] + } +)); + +const inValidWithNeverOption = codeWithGetters.map((code) => { + const options = ['never']; + return { + code, + parserOptions, + options, + output: null, + errors: [{ + message: rule.meta.message, + }] + }; +}); + +const inValidWithAlwaysOption = codeWithoutGetters.map((code) => { + const options = ['always']; + return { + code, + parserOptions, + options, + output: null, + errors: [{ + message: rule.meta.message, + }] + }; +}); + +ruleTester.run('computed-property-getters', rule, { + valid: [...validWithDefaultOptions, ...validWithNeverOption, ...validWithAlwaysOption], + invalid: [...inValidWithDefaultOptions, ...inValidWithNeverOption, ...inValidWithAlwaysOption], +}); From 1dac868b09d1ea8297254778c87cf21fd96a2a1b Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Mon, 15 Apr 2019 15:34:30 -0700 Subject: [PATCH 2/3] Change default for computed-property-getters This defaults to requiring getters if there is a setter. It preserves the option to prevent getters entirely as a proxy for preventing setters as well. --- docs/rules/computed-property-getters.md | 53 +++++++++++++-- lib/rules/computed-property-getters.js | 27 +++++++- tests/lib/rules/computed-property-getters.js | 70 +++++++++++++++++++- 3 files changed, 141 insertions(+), 9 deletions(-) diff --git a/docs/rules/computed-property-getters.md b/docs/rules/computed-property-getters.md index b79fc48931..de6c5d98a4 100644 --- a/docs/rules/computed-property-getters.md +++ b/docs/rules/computed-property-getters.md @@ -11,22 +11,36 @@ This rule takes a single string option String option: -* `"never"` (default) getters are *never allowed* in computed properties +* `"always-with-setter"` (default) getters are *required* when computed property has a setter * `"always"` getters are *required* in computed properties +* `"never"` getters are *never allowed* in computed properties -### never + +### always-with-setter ```javascript -/// GOOD +/// BAD +Ember.Object.extend({ + fullName: computed('firstName', 'lastName', { + get() { + //... + } + }) +}); + +// GOOD Ember.Object.extend({ fullName: computed('firstName', 'lastName', function() { //... }) }); -// BAD +// GOOD Ember.Object.extend({ fullName: computed('firstName', 'lastName', { + set() { + //... + }, get() { //... } @@ -34,6 +48,7 @@ Ember.Object.extend({ }); ``` + ### always ```javascript @@ -54,4 +69,34 @@ Ember.Object.extend({ }); ``` +### never + +```javascript +/// GOOD +Ember.Object.extend({ + fullName: computed('firstName', 'lastName', function() { + //... + }) +}); + +// BAD +Ember.Object.extend({ + fullName: computed('firstName', 'lastName', { + get() { + //... + } + }) +}); +// BAD +Ember.Object.extend({ + fullName: computed('firstName', 'lastName', { + get() { + //... + }, + set() { + //... + } + }) +}); +``` diff --git a/lib/rules/computed-property-getters.js b/lib/rules/computed-property-getters.js index 78e0828019..f3131505fa 100644 --- a/lib/rules/computed-property-getters.js +++ b/lib/rules/computed-property-getters.js @@ -18,19 +18,36 @@ module.exports = { fixable: null, // or "code" or "whitespace" schema: [ { - enum: ['always', 'never'] + enum: ['always-with-setter', 'always', 'never'] }, ], }, create(context) { - const requireGetters = context.options[0] || 'never'; + const requireGetters = context.options[0] || 'always-with-setter'; const message = 'Do not use a getter inside computed properties.'; const report = function (node) { context.report(node, message); }; + const requireGetterWithSetterInComputedProperty = function (node) { + const objectExpressions = node.arguments.filter(arg => utils.isObjectExpression(arg)); + if ( + objectExpressions.length + ) { + const { properties } = objectExpressions[0]; + const getters = properties.filter(prop => prop.key && prop.key.name && prop.key.name === 'get'); + const setters = properties.filter(prop => prop.key && prop.key.name && prop.key.name === 'set'); + if ( + setters.length === 0 || + (setters.length > 0 && getters.length === 0) + ) { + report(node); + } + } + }; + const preventGetterInComputedProperty = function (node) { const objectExpressions = node.arguments.filter(arg => utils.isObjectExpression(arg)); if ( @@ -55,9 +72,13 @@ module.exports = { ember.isComputedProp(node) && node.arguments.length ) { + if (requireGetters === 'always-with-setter') { + requireGetterWithSetterInComputedProperty(node); + } if (requireGetters === 'always') { requireGetterInComputedProperty(node); - } else { + } + if (requireGetters === 'never') { preventGetterInComputedProperty(node); } } diff --git a/tests/lib/rules/computed-property-getters.js b/tests/lib/rules/computed-property-getters.js index facee06124..29a1748de0 100644 --- a/tests/lib/rules/computed-property-getters.js +++ b/tests/lib/rules/computed-property-getters.js @@ -70,8 +70,69 @@ const codeWithGetters = [ }` ]; +const codeWithSettersAndGetters = [ + `{ + foo: computed({ + get() { + return true; + }, + set() { + return true; + } + }).readonly() + }`, + `{ + foo: computed({ + get() { + return true; + }, + set() { + return true; + } + }) + }`, + `{ + foo: computed('model.foo', { + get() { + return true; + }, + set() { + return true; + } + }).readonly() + }`, + `{ + foo: computed('model.foo', { + get() { + return true; + }, + set() { + return true; + } + }) + }`, + `{ + foo: computed('model.foo', { + get() {}, + set() {}, + }) + }` +]; + + +const validWithDefaultOptions = []; +validWithDefaultOptions.push(...codeWithoutGetters.map(code => ({ code, parserOptions }))); +validWithDefaultOptions.push(...codeWithSettersAndGetters.map(code => ({ code, parserOptions }))); -const validWithDefaultOptions = codeWithoutGetters.map(code => ({ code, parserOptions })); +const validWithAlwaysWithSetterOptions = []; +validWithAlwaysWithSetterOptions.push(...codeWithoutGetters.map((code) => { + const options = ['always-with-setter']; + return { code, parserOptions, options }; +})); +validWithAlwaysWithSetterOptions.push(...codeWithSettersAndGetters.map((code) => { + const options = ['always-with-setter']; + return { code, parserOptions, options }; +})); const validWithNeverOption = codeWithoutGetters.map((code) => { const options = ['never']; @@ -121,6 +182,11 @@ const inValidWithAlwaysOption = codeWithoutGetters.map((code) => { }); ruleTester.run('computed-property-getters', rule, { - valid: [...validWithDefaultOptions, ...validWithNeverOption, ...validWithAlwaysOption], + valid: [ + ...validWithDefaultOptions, + ...validWithAlwaysWithSetterOptions, + ...validWithNeverOption, + ...validWithAlwaysOption + ], invalid: [...inValidWithDefaultOptions, ...inValidWithNeverOption, ...inValidWithAlwaysOption], }); From 1a77d94960cc6efade68cbbe1a77cc7f70a94288 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Tue, 7 May 2019 15:19:55 -0700 Subject: [PATCH 3/3] Prevent Lonely Setters A setter should never be present when there are no getters no matter what other options are selected. --- lib/rules/computed-property-getters.js | 25 ++-- tests/lib/rules/computed-property-getters.js | 122 +++++++++++++------ 2 files changed, 101 insertions(+), 46 deletions(-) diff --git a/lib/rules/computed-property-getters.js b/lib/rules/computed-property-getters.js index f3131505fa..dcf6f416ea 100644 --- a/lib/rules/computed-property-getters.js +++ b/lib/rules/computed-property-getters.js @@ -31,7 +31,7 @@ module.exports = { context.report(node, message); }; - const requireGetterWithSetterInComputedProperty = function (node) { + const requireGetterOnlyWithASetterInComputedProperty = function (node) { const objectExpressions = node.arguments.filter(arg => utils.isObjectExpression(arg)); if ( objectExpressions.length @@ -40,8 +40,8 @@ module.exports = { const getters = properties.filter(prop => prop.key && prop.key.name && prop.key.name === 'get'); const setters = properties.filter(prop => prop.key && prop.key.name && prop.key.name === 'set'); if ( - setters.length === 0 || - (setters.length > 0 && getters.length === 0) + (setters.length > 0 && getters.length === 0) || + (getters.length > 0 && setters.length === 0) ) { report(node); } @@ -53,15 +53,26 @@ module.exports = { if ( objectExpressions.length ) { - report(node); + const { properties } = objectExpressions[0]; + const getters = properties.filter(prop => prop.key && prop.key.name && prop.key.name === 'get'); + const setters = properties.filter(prop => prop.key && prop.key.name && prop.key.name === 'set'); + if (getters.length > 0 || setters.length > 0) { + report(node); + } } }; const requireGetterInComputedProperty = function (node) { - const functionExpressions = node.arguments.filter(arg => utils.isFunctionExpression(arg)); + const objectExpressions = node.arguments.filter(arg => utils.isObjectExpression(arg)); if ( - functionExpressions.length + objectExpressions.length ) { + const { properties } = objectExpressions[0]; + const getters = properties.filter(prop => prop.key && prop.key.name && prop.key.name === 'get'); + if (getters.length === 0) { + report(node); + } + } else { report(node); } }; @@ -73,7 +84,7 @@ module.exports = { node.arguments.length ) { if (requireGetters === 'always-with-setter') { - requireGetterWithSetterInComputedProperty(node); + requireGetterOnlyWithASetterInComputedProperty(node); } if (requireGetters === 'always') { requireGetterInComputedProperty(node); diff --git a/tests/lib/rules/computed-property-getters.js b/tests/lib/rules/computed-property-getters.js index 29a1748de0..60085065a4 100644 --- a/tests/lib/rules/computed-property-getters.js +++ b/tests/lib/rules/computed-property-getters.js @@ -14,8 +14,12 @@ const RuleTester = require('eslint').RuleTester; const ruleTester = new RuleTester(); const parserOptions = { ecmaVersion: 2018, sourceType: 'module' }; +const errors = [{ + message: rule.meta.message, +}]; +const output = null; -const codeWithoutGetters = [ +const codeWithoutGettersOrSetters = [ ` { foo: computed('model', function() {}) @@ -34,7 +38,7 @@ const codeWithoutGetters = [ }` ]; -const codeWithGetters = [ +const codeWithOnlyGetters = [ `{ foo: computed({ get() { @@ -70,6 +74,42 @@ const codeWithGetters = [ }` ]; +const codeWithOnlySetters = [ + `{ + foo: computed({ + set() { + return true; + } + }).readonly() + }`, + `{ + foo: computed({ + set() { + return true; + } + }) + }`, + `{ + foo: computed('model.foo', { + set() { + return true; + } + }).readonly() + }`, + `{ + foo: computed('model.foo', { + set() { + return true; + } + }) + }`, + `{ + foo: computed('model.foo', { + set() {} + }) + }` +]; + const codeWithSettersAndGetters = [ `{ foo: computed({ @@ -121,11 +161,11 @@ const codeWithSettersAndGetters = [ const validWithDefaultOptions = []; -validWithDefaultOptions.push(...codeWithoutGetters.map(code => ({ code, parserOptions }))); +validWithDefaultOptions.push(...codeWithoutGettersOrSetters.map(code => ({ code, parserOptions }))); validWithDefaultOptions.push(...codeWithSettersAndGetters.map(code => ({ code, parserOptions }))); const validWithAlwaysWithSetterOptions = []; -validWithAlwaysWithSetterOptions.push(...codeWithoutGetters.map((code) => { +validWithAlwaysWithSetterOptions.push(...codeWithoutGettersOrSetters.map((code) => { const options = ['always-with-setter']; return { code, parserOptions, options }; })); @@ -134,52 +174,52 @@ validWithAlwaysWithSetterOptions.push(...codeWithSettersAndGetters.map((code) => return { code, parserOptions, options }; })); -const validWithNeverOption = codeWithoutGetters.map((code) => { +const validWithNeverOption = codeWithoutGettersOrSetters.map((code) => { const options = ['never']; return { code, parserOptions, options }; }); -const validWithAlwaysOption = codeWithGetters.map((code) => { +const validWithAlwaysOption = []; +validWithAlwaysOption.push(...codeWithOnlyGetters.map((code) => { const options = ['always']; return { code, parserOptions, options }; -}); +})); +validWithAlwaysOption.push(...codeWithSettersAndGetters.map((code) => { + const options = ['always']; + return { code, parserOptions, options }; +})); -const inValidWithDefaultOptions = codeWithGetters.map(code => ( - { - code, - parserOptions, - output: null, - errors: [{ - message: rule.meta.message, - }] - } +const inValidWithDefaultOptions = []; +inValidWithDefaultOptions.push(...codeWithOnlyGetters.map(code => + ({ code, parserOptions, output, errors }) +)); +inValidWithDefaultOptions.push(...codeWithOnlySetters.map(code => + ({ code, parserOptions, output, errors }) )); -const inValidWithNeverOption = codeWithGetters.map((code) => { +const inValidWithNeverOption = []; +inValidWithNeverOption.push(...codeWithOnlyGetters.map((code) => { const options = ['never']; - return { - code, - parserOptions, - options, - output: null, - errors: [{ - message: rule.meta.message, - }] - }; -}); + return { code, parserOptions, options, output, errors }; +})); +inValidWithNeverOption.push(...codeWithOnlySetters.map((code) => { + const options = ['never']; + return { code, parserOptions, options, output, errors }; +})); +inValidWithNeverOption.push(...codeWithSettersAndGetters.map((code) => { + const options = ['never']; + return { code, parserOptions, options, output, errors }; +})); -const inValidWithAlwaysOption = codeWithoutGetters.map((code) => { +const inValidWithAlwaysOption = []; +inValidWithAlwaysOption.push(...codeWithoutGettersOrSetters.map((code) => { const options = ['always']; - return { - code, - parserOptions, - options, - output: null, - errors: [{ - message: rule.meta.message, - }] - }; -}); + return { code, parserOptions, options, output, errors }; +})); +inValidWithAlwaysOption.push(...codeWithOnlySetters.map((code) => { + const options = ['always']; + return { code, parserOptions, options, output, errors }; +})); ruleTester.run('computed-property-getters', rule, { valid: [ @@ -188,5 +228,9 @@ ruleTester.run('computed-property-getters', rule, { ...validWithNeverOption, ...validWithAlwaysOption ], - invalid: [...inValidWithDefaultOptions, ...inValidWithNeverOption, ...inValidWithAlwaysOption], + invalid: [ + ...inValidWithDefaultOptions, + ...inValidWithNeverOption, + ...inValidWithAlwaysOption + ], });