From a8b7a683ae2f75ffd50986b2c57c40533a630db3 Mon Sep 17 00:00:00 2001 From: Evan Farina Date: Thu, 2 Nov 2017 10:44:25 -0400 Subject: [PATCH 1/4] Refactored and updated the rule to account for more lifecycle hooks and more object types --- lib/rules/no-broken-super-chain.js | 148 +++++++++++++---------- tests/lib/rules/no-broken-super-chain.js | 119 ++++++++++++------ 2 files changed, 165 insertions(+), 102 deletions(-) diff --git a/lib/rules/no-broken-super-chain.js b/lib/rules/no-broken-super-chain.js index 7f83f89..08b22ce 100644 --- a/lib/rules/no-broken-super-chain.js +++ b/lib/rules/no-broken-super-chain.js @@ -3,93 +3,107 @@ */ const MESSAGES = { - noSuper: '\'this._super(...arguments)\' must be called in init()', - noThisBeforeSuper: 'Must call \'this._super(...arguments)\' before accessing `this`', - tooManySupers: 'Only call `this._super(...arguments)` once per init()' + noSuper: '`this._super(...arguments)` must be called in', + tooManySupers: 'Only call `this._super(...arguments)` once per lifecycle hook', + argsNotPassedToSuper: '...arguments need to be passed to this._super() call' }; -// TODO: Make this configurable -const EMBER_MODULES_WITH_SUPER_CHAIN = { - Component: true, - Mixin: true, - Route: true, - Controller: true, - View: true -}; +const LIFECYCLE_HOOKS = ['init', 'willDestroy', 'willDestroyElement', 'destroy']; -/** - * Determines if this is an init method in an extension of Ember[EMBER_MODULES_WITH_SUPER_CHAIN.*] - * @param {Node} node - */ -function isInit(node) { - if (node.type === 'FunctionExpression' && node.parent && node.parent.key && node.parent.key.name === 'init') { +function isExtend(node) { + return node && node.callee && node.callee.property && node.callee.property.name === 'extend'; +} + +function isSuperCall(lineWithinFn) { + if (lineWithinFn.type !== 'MemberExpression') { + return false; + } + + let isSuperCall = false; + + if (lineWithinFn.object.type === 'ThisExpression') { + isSuperCall = lineWithinFn.property.type === 'Identifier' && lineWithinFn.property.name === '_super'; + } else if (lineWithinFn.object.type === 'MemberExpression') { + isSuperCall = lineWithinFn.object.property.name === '_super'; + } + + return isSuperCall; +} - if (node.parent.parent - && node.parent.parent.parent - && node.parent.parent.parent.callee - && node.parent.parent.parent.callee.object - && node.parent.parent.parent.callee.object.object - && node.parent.parent.parent.callee.object.object.name === 'Ember') { - return (node.parent.parent.parent.callee.object.property - && EMBER_MODULES_WITH_SUPER_CHAIN[node.parent.parent.parent.callee.object.property.name]); - } +function wereArgumentsPassedToSuper(expression) { + const callee = expression.callee; + + if (!expression || callee.type !== 'MemberExpression') { + return; } - return false; + if (callee.property.name === '_super') { + const firstArgumentToSuper = expression.arguments[0]; + return firstArgumentToSuper && firstArgumentToSuper.type === 'SpreadElement' && firstArgumentToSuper.argument.name === 'arguments'; + } else if (callee.property.name === 'apply') { + const args = expression.arguments; + return args.length >= 2 && args[0].type === 'ThisExpression' && args[1].name === 'arguments'; + } } module.exports = { meta: { docs: { - description: 'Prevent the absence of `this._super(...arguments)` in `init()` calls or the use of `this` prior to `this._super()`', + description: 'Prevent the absence of `this._super(...arguments)` in calls to various lifecycle hooks or the use of `this` prior to `this._super()`', category: 'Best Practices', recommended: true }, messages: MESSAGES }, create(context) { - let initOverride = null; - return { - onCodePathStart(codePath, node) { - if (isInit(node)) { - initOverride = { - superCalled: false, - superCalledFirst: false - }; - } - }, - onCodePathEnd(codePath, node) { - if (initOverride && isInit(node)) { // TODO: Maybe check against codepath.name - if (!initOverride.superCalled) { - context.report({ - message: MESSAGES.noSuper, - node - }); - } + CallExpression(node) { + if (isExtend(node)) { + let superCount = 0; + const extendedObjects = node.arguments; - initOverride = null; - } - return; - }, - 'CallExpression:exit'(node) { - if (initOverride) { - const property = node.callee.property; - if (property && property.type === 'Identifier' && property.name === '_super') { - if (initOverride.superCalled) { - context.report({ - message: MESSAGES.tooManySupers, - node - }); - } else { - initOverride.superCalled = true; - } - } + extendedObjects.forEach(extendedObj => { + extendedObj.properties.forEach(property => { + if (LIFECYCLE_HOOKS.includes(property.key.name)) { + const propertyFnBody = property.value.body; + + if (propertyFnBody && propertyFnBody.body) { + let expression; + property.value.body.body.forEach(fnBody => { + expression = fnBody.expression; + if (expression.type === 'CallExpression') { + const line = expression.callee; + if (isSuperCall(line)) { + if (!wereArgumentsPassedToSuper(expression)) { + context.report({ + node: fnBody, + message: context.meta.messages.argsNotPassedToSuper + }); + } + superCount++; + } + } + }); + + if (superCount === 0) { + context.report({ + node: property, + message: `${context.meta.messages.noSuper} ${property.key.name}` + }); + } else if (superCount > 1) { + context.report({ + node: property, + message: context.meta.messages.tooManySupers + }); + } + + superCount = 0; + } + } + }); + }); } - }, - 'Program:exit'() { - initOverride = null; } }; } -}; \ No newline at end of file +}; diff --git a/tests/lib/rules/no-broken-super-chain.js b/tests/lib/rules/no-broken-super-chain.js index 59d9cfe..569eb7b 100644 --- a/tests/lib/rules/no-broken-super-chain.js +++ b/tests/lib/rules/no-broken-super-chain.js @@ -1,7 +1,7 @@ const rule = require('../../../lib/rules/no-broken-super-chain'); const RuleTester = require('eslint').RuleTester; -const { noSuper, tooManySupers } = rule.meta.messages; +const { noSuper, tooManySupers, argsNotPassedToSuper } = rule.meta.messages; const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6, @@ -23,6 +23,15 @@ ruleTester.run('no-broken-super-chain', rule, { } });` }, + { + code: ` + export default Ember.Component.extend({ + init() { + this._super.apply(this, arguments); + this.alias = this.concrete; + }, + });` + }, { code: ` export default Ember.Route.extend({ @@ -47,14 +56,6 @@ ruleTester.run('no-broken-super-chain', rule, { } });` }, - { - code: ` - export default Ember.Service.extend({ - init() { - this.alias = this.concrete; - } - });` - }, { code: ` export default Ember.Component.extend({ @@ -85,6 +86,17 @@ ruleTester.run('no-broken-super-chain', rule, { } ], invalid: [ + { + code: ` + export default Ember.Service.extend({ + init() { + this.alias = this.concrete; + } + });`, + errors: [{ + message: `${noSuper} init` + }] + }, { code: ` export default Ember.Component.extend({ @@ -93,7 +105,40 @@ ruleTester.run('no-broken-super-chain', rule, { } });`, errors: [{ - message: noSuper + message: `${noSuper} init` + }] + }, + { + code: ` + export default Ember.Component.extend({ + destroy() { + this.alias = this.concrete; + } + });`, + errors: [{ + message: `${noSuper} destroy` + }] + }, + { + code: ` + export default Ember.Component.extend({ + willDestroy() { + this.alias = this.concrete; + } + });`, + errors: [{ + message: `${noSuper} willDestroy` + }] + }, + { + code: ` + export default Ember.Component.extend({ + willDestroyElement() { + this.alias = this.concrete; + } + });`, + errors: [{ + message: `${noSuper} willDestroyElement` }] }, { @@ -104,32 +149,36 @@ ruleTester.run('no-broken-super-chain', rule, { } });`, errors: [{ - message: noSuper + message: `${noSuper} init` }] }, - // TODO - // { - // code: ` - // export default Ember.Component.extend({ - // init() { - // this._super(); // missing '...arguments' - // this.alias = this.concrete; - // } - // });`, - // }, - // TODO - // { - // code: ` - // export default Ember.Component.extend({ - // init() { - // this.alias = this.concrete; - // this._super(...arguments); - // } - // });`, - // errors: [{ - // message: noThisBeforeSuper - // }] - // }, + { + code: ` + export default Ember.Component.extend({ + init() { + this._super(); // missing '...arguments' + this.alias = this.concrete; + } + });`, + errors: [{ + message: argsNotPassedToSuper + }] + }, + /** + TODO + { + code: ` + export default Ember.Component.extend({ + init() { + this.alias = this.concrete; + this._super(...arguments); + } + });`, + errors: [{ + message: noThisBeforeSuper + }] + }, + */ { code: ` export default Ember.Component.extend({ @@ -156,4 +205,4 @@ ruleTester.run('no-broken-super-chain', rule, { // }] // } ] -}); \ No newline at end of file +}); From abdad6de78c56d36f0fc83c1e04bcc69181a5523 Mon Sep 17 00:00:00 2001 From: Evan Farina Date: Sat, 4 Nov 2017 10:52:47 -0400 Subject: [PATCH 2/4] Addressed feedback, added tests for new cases --- lib/rules/no-broken-super-chain.js | 23 ++++++++++------------- tests/lib/rules/no-broken-super-chain.js | 16 ++++++++++++++++ 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/lib/rules/no-broken-super-chain.js b/lib/rules/no-broken-super-chain.js index 08b22ce..7e5a523 100644 --- a/lib/rules/no-broken-super-chain.js +++ b/lib/rules/no-broken-super-chain.js @@ -1,5 +1,5 @@ /** - * @fileOverview Prevent the absence of this._super() in init() calls or the use of this prior to this._super() + * @fileOverview Prevent the absence of `this._super(...arguments)` in calls to various lifecycle hooks */ const MESSAGES = { @@ -42,14 +42,14 @@ function wereArgumentsPassedToSuper(expression) { return firstArgumentToSuper && firstArgumentToSuper.type === 'SpreadElement' && firstArgumentToSuper.argument.name === 'arguments'; } else if (callee.property.name === 'apply') { const args = expression.arguments; - return args.length >= 2 && args[0].type === 'ThisExpression' && args[1].name === 'arguments'; + return args.length === 2 && args[0].type === 'ThisExpression' && args[1].name === 'arguments'; } } module.exports = { meta: { docs: { - description: 'Prevent the absence of `this._super(...arguments)` in calls to various lifecycle hooks or the use of `this` prior to `this._super()`', + description: 'Prevent the absence of `this._super(...arguments)` in calls to various lifecycle hooks', category: 'Best Practices', recommended: true }, @@ -59,24 +59,23 @@ module.exports = { return { CallExpression(node) { if (isExtend(node)) { - let superCount = 0; - const extendedObjects = node.arguments; - + const extendedObjects = node.arguments.filter(arg => arg.type === 'ObjectExpression'); extendedObjects.forEach(extendedObj => { + let superCount = 0; extendedObj.properties.forEach(property => { if (LIFECYCLE_HOOKS.includes(property.key.name)) { const propertyFnBody = property.value.body; if (propertyFnBody && propertyFnBody.body) { let expression; - property.value.body.body.forEach(fnBody => { - expression = fnBody.expression; + propertyFnBody.body.forEach(expressionStatement => { + expression = expressionStatement.type === 'ReturnStatement' ? expressionStatement.argument : expressionStatement.expression; if (expression.type === 'CallExpression') { - const line = expression.callee; - if (isSuperCall(line)) { + const callee = expression.callee; + if (isSuperCall(callee)) { if (!wereArgumentsPassedToSuper(expression)) { context.report({ - node: fnBody, + node: expressionStatement, message: context.meta.messages.argsNotPassedToSuper }); } @@ -96,8 +95,6 @@ module.exports = { message: context.meta.messages.tooManySupers }); } - - superCount = 0; } } }); diff --git a/tests/lib/rules/no-broken-super-chain.js b/tests/lib/rules/no-broken-super-chain.js index 569eb7b..c71038e 100644 --- a/tests/lib/rules/no-broken-super-chain.js +++ b/tests/lib/rules/no-broken-super-chain.js @@ -83,6 +83,22 @@ ruleTester.run('no-broken-super-chain', rule, { }); export default foo;` + }, + { + code: ` + export default Ember.Component.extend(SomeMixin, { + init() { + this._super(...arguments); + } + });` + }, + { + code: ` + export default Ember.Component.extend({ + init() { + return this._super(...arguments); + } + });` } ], invalid: [ From 56dcd1bdc0d3ee6585bea556a3bebed8c54a9b5c Mon Sep 17 00:00:00 2001 From: Evan Farina Date: Sat, 4 Nov 2017 10:52:47 -0400 Subject: [PATCH 3/4] Addressed feedback, added tests for new cases --- tests/lib/rules/no-broken-super-chain.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/lib/rules/no-broken-super-chain.js b/tests/lib/rules/no-broken-super-chain.js index c71038e..5dda187 100644 --- a/tests/lib/rules/no-broken-super-chain.js +++ b/tests/lib/rules/no-broken-super-chain.js @@ -99,6 +99,10 @@ ruleTester.run('no-broken-super-chain', rule, { return this._super(...arguments); } });` + }, + { + code: ` + export default Foo.extend({ [lol]: function() {}});` } ], invalid: [ From 1a8aad33730186c28d1d53e5791e633b2129322c Mon Sep 17 00:00:00 2001 From: Evan Farina Date: Sat, 4 Nov 2017 11:29:12 -0400 Subject: [PATCH 4/4] Added more lifecycle hooks --- lib/rules/no-broken-super-chain.js | 16 +- tests/lib/rules/no-broken-super-chain.js | 212 +++++++++++++++++++---- 2 files changed, 197 insertions(+), 31 deletions(-) diff --git a/lib/rules/no-broken-super-chain.js b/lib/rules/no-broken-super-chain.js index 7e5a523..b7898db 100644 --- a/lib/rules/no-broken-super-chain.js +++ b/lib/rules/no-broken-super-chain.js @@ -8,7 +8,21 @@ const MESSAGES = { argsNotPassedToSuper: '...arguments need to be passed to this._super() call' }; -const LIFECYCLE_HOOKS = ['init', 'willDestroy', 'willDestroyElement', 'destroy']; +const LIFECYCLE_HOOKS = [ + 'init', + 'didReceiveAttrs', + 'willRender', + 'didInsertElement', + 'didRender', + 'didUpdateAttrs', + 'willUpdate', + 'didUpdate', + 'willDestroy', + 'willDestroyElement', + 'willClearRender', + 'destroy', + 'didDestroyElement' +]; function isExtend(node) { return node && node.callee && node.callee.property && node.callee.property.name === 'extend'; diff --git a/tests/lib/rules/no-broken-super-chain.js b/tests/lib/rules/no-broken-super-chain.js index 5dda187..fcae935 100644 --- a/tests/lib/rules/no-broken-super-chain.js +++ b/tests/lib/rules/no-broken-super-chain.js @@ -56,14 +56,6 @@ ruleTester.run('no-broken-super-chain', rule, { } });` }, - { - code: ` - export default Ember.Component.extend({ - didInsertElement() { - this.updateBlurHandler(true); - } - });` - }, { code: ` export default MyComponent.extend({ @@ -100,6 +92,94 @@ ruleTester.run('no-broken-super-chain', rule, { } });` }, + { + code: ` + export default Ember.Component.extend({ + didReceiveAttrs() { + return this._super(...arguments); + } + });` + }, + { + code: ` + export default Ember.Component.extend({ + willRender() { + return this._super(...arguments); + } + });` + }, + { + code: ` + export default Ember.Component.extend({ + didInsertElement() { + return this._super(...arguments); + } + });` + }, + { + code: ` + export default Ember.Component.extend({ + didRender() { + return this._super(...arguments); + } + });` + }, + { + code: ` + export default Ember.Component.extend({ + didUpdateAttrs() { + return this._super(...arguments); + } + });` + }, + { + code: ` + export default Ember.Component.extend({ + willUpdate() { + return this._super(...arguments); + } + });` + }, + { + code: ` + export default Ember.Component.extend({ + didUpdate() { + return this._super(...arguments); + } + });` + }, + { + code: ` + export default Ember.Component.extend({ + willDestroy() { + return this._super(...arguments); + } + });` + }, + { + code: ` + export default Ember.Component.extend({ + willDestroyElement() { + return this._super(...arguments); + } + });` + }, + { + code: ` + export default Ember.Component.extend({ + willClearRender() { + return this._super(...arguments); + } + });` + }, + { + code: ` + export default Ember.Component.extend({ + didDestroyElement() { + return this._super(...arguments); + } + });` + }, { code: ` export default Foo.extend({ [lol]: function() {}});` @@ -184,45 +264,117 @@ ruleTester.run('no-broken-super-chain', rule, { message: argsNotPassedToSuper }] }, - /** - TODO { code: ` export default Ember.Component.extend({ init() { + this._super(...arguments); this.alias = this.concrete; this._super(...arguments); } });`, errors: [{ - message: noThisBeforeSuper + message: tooManySupers }] }, - */ { code: ` - export default Ember.Component.extend({ - init() { - this._super(...arguments); - this.alias = this.concrete; - this._super(...arguments); + export default MyComponent.extend({ + didReceiveAttrs() { + this.someMethod(); } });`, errors: [{ - message: tooManySupers + message: `${noSuper} didReceiveAttrs` + }] + }, + { + code: ` + export default MyComponent.extend({ + willRender() { + this.someMethod(); + } + });`, + errors: [{ + message: `${noSuper} willRender` + }] + }, + { + code: ` + export default MyComponent.extend({ + didInsertElement() { + this.someMethod(); + } + });`, + errors: [{ + message: `${noSuper} didInsertElement` + }] + }, + { + code: ` + export default MyComponent.extend({ + didRender() { + this.someMethod(); + } + });`, + errors: [{ + message: `${noSuper} didRender` + }] + }, + { + code: ` + export default MyComponent.extend({ + didUpdateAttrs() { + this.someMethod(); + } + });`, + errors: [{ + message: `${noSuper} didUpdateAttrs` + }] + }, + { + code: ` + export default MyComponent.extend({ + willUpdate() { + this.someMethod(); + } + });`, + errors: [{ + message: `${noSuper} willUpdate` + }] + }, + { + code: ` + export default MyComponent.extend({ + didUpdate() { + this.someMethod(); + } + });`, + errors: [{ + message: `${noSuper} didUpdate` + }] + }, + { + code: ` + export default MyComponent.extend({ + willClearRender() { + this.someMethod(); + } + });`, + errors: [{ + message: `${noSuper} willClearRender` + }] + }, + { + code: ` + export default MyComponent.extend({ + didDestroyElement() { + this.someMethod(); + } + });`, + errors: [{ + message: `${noSuper} didDestroyElement` }] } - // TODO - // { - // code: ` - // export default MyComponent.extend({ - // didInsertElement() { - // this.updateBlurHandler(true); - // } - // });`, - // errors: [{ - // message: noSuper - // }] - // } ] });