Updated `no-broken-super-chain` rule to account for more lifecycle hooks … #94

evanfarina · 2017-11-02T14:47:30Z

…and more object types. The rule now checks any object that's using extend, and validates the init, willDestroy, destroy and willDestroyElement hooks. In addition, I updated the rule to validate that arguments were correctly passed to the super call. Both of the below pass the rule:

this._super(...arguments);
this._super.apply(this, arguments)

…nd more object types

rwjblue

Looking good! I left some notes inline with suggested cleanups to make, and I think we need a few more test cases to cover those things.

I'd suggest the following additional test cases:

valid
- Foo.extend(SomeMixin)
- Foo.extend(SomeMixin, { init() { this._super(...arguments) } })
- Foo.extend({ [lol]: function() {})

rwjblue · 2017-11-03T13:26:22Z

lib/rules/no-broken-super-chain.js

-            }
-          }
+          extendedObjects.forEach(extendedObj => {
+            extendedObj.properties.forEach(property => {


We need to guard for extendedObj.properties being undefined here (or filter out non ObjectExpressions before .forEaching).

I believe the following test should be added (which will fail ATM but pass if we filtered non ObjectExpressions):

Foo.extend(SomeMixin, { init() { this._super(...arguments); } });

I'd suggest either of the following:

extendedObjects = node.arguments.filter(arg => arg.type === 'ObjectExpression'); // or extendedObjects.forEach(extendedObj => { if (extendedObj.type !== 'ObjectExpression') { return; } // ...snip... });

I would also suggest adding a test case for this.

Updated, and added test.

rwjblue · 2017-11-03T13:34:47Z

lib/rules/no-broken-super-chain.js

-          }
+      CallExpression(node) {
+        if (isExtend(node)) {
+          let superCount = 0;


Seems like this variable should be initialized from inside the extendedObjects.forEach loop (then you wouldn't need to reset it to 0 at the end of the loop).

rwjblue · 2017-11-03T13:35:36Z

lib/rules/no-broken-super-chain.js

+
+                if (propertyFnBody && propertyFnBody.body) {
+                  let expression;
+                  property.value.body.body.forEach(fnBody => {


This should likely use propertyFnBody.body instead of property.value.body.body.forEach

Can you rename fnBody to expressionStatement?

Correct. 2. Gladly

rwjblue · 2017-11-03T13:37:40Z

lib/rules/no-broken-super-chain.js

+                  property.value.body.body.forEach(fnBody => {
+                    expression = fnBody.expression;
+                    if (expression.type === 'CallExpression') {
+                      const line = expression.callee;


line isn't super descriptive, maybe call this callee?

rwjblue · 2017-11-03T13:38:40Z

lib/rules/no-broken-super-chain.js

+                    });
+                  }
+
+                  superCount = 0;


This shouldn't be needed, we should initialize superCount inside the loop...

rwjblue · 2017-11-03T13:40:24Z

lib/rules/no-broken-super-chain.js

+    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';


When would args be > 2? I think we want args.length === 2...

Yeah, not sure what I had in mind here

rwjblue · 2017-11-03T13:41:13Z

lib/rules/no-broken-super-chain.js

 }

 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()`',


or the use of this prior to this._super()

I don't think we do this at the moment, do we? I don't see the code that does it at least...

good catch, also updated file description

mdebbar · 2017-11-03T16:42:58Z

lib/rules/no-broken-super-chain.js

-  Controller: true,
-  View: true
-};
+const LIFECYCLE_HOOKS = ['init', 'willDestroy', 'willDestroyElement', 'destroy'];


@rwjblue to be 100% safe, I think we should add all life cycle hooks here. Otherwise, we have no guarantee that mixins will work as expected.

Ya, makes sense. Do we have a canonical list?

I guess we can start with the ones listed in https://guides.emberjs.com/v2.16.0/components/the-component-lifecycle/...

mdebbar · 2017-11-03T16:43:27Z

lib/rules/no-broken-super-chain.js

+    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';


mdebbar · 2017-11-03T16:46:12Z

lib/rules/no-broken-super-chain.js

-            }
-          }
+          extendedObjects.forEach(extendedObj => {
+            extendedObj.properties.forEach(property => {


I would also suggest adding a test case for this.

mdebbar · 2017-11-03T16:52:22Z

lib/rules/no-broken-super-chain.js

+  return node && node.callee && node.callee.property && node.callee.property.name === 'extend';
+}
+
+function isSuperCall(lineWithinFn) {


One more case we need to handle is:

Foo.extend({ init() { return this._super(...argument); } });

This ^ is valid, the linting rule should not complain about it.

Great catch. Updated to: expression = expressionStatement.type === 'ReturnStatement' ? expressionStatement.argument : expressionStatement.expression;

rwjblue · 2017-11-04T15:11:46Z

tests/lib/rules/no-broken-super-chain.js

+          }
+        });`,
+      errors: [{
+        message: noThisBeforeSuper


Let’s remove this TODO commented test.

When we add a rule about no this before super we will probably make it a new more focused rule anyways.

Removed TODO

rwjblue · 2017-11-04T15:14:27Z

lib/rules/no-broken-super-chain.js

-  Controller: true,
-  View: true
-};
+const LIFECYCLE_HOOKS = ['init', 'willDestroy', 'willDestroyElement', 'destroy'];


Ya, makes sense. Do we have a canonical list?

I guess we can start with the ones listed in https://guides.emberjs.com/v2.16.0/components/the-component-lifecycle/...

Refactored and updated the rule to account for more lifecycle hooks a…

a8b7a68

…nd more object types

evanfarina changed the title ~~Updated no-broken-super-chain rule[ to account for more lifecycle hooks …~~ Updated no-broken-super-chain rule to account for more lifecycle hooks … Nov 2, 2017

rwjblue reviewed Nov 3, 2017

View reviewed changes

mdebbar suggested changes Nov 3, 2017

View reviewed changes

evanfarina added 2 commits November 4, 2017 10:52

Addressed feedback, added tests for new cases

abdad6d

Addressed feedback, added tests for new cases

56dcd1b

rwjblue reviewed Nov 4, 2017

View reviewed changes

Added more lifecycle hooks

1a8aad3

rwjblue approved these changes Nov 4, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated `no-broken-super-chain` rule to account for more lifecycle hooks … #94

Updated `no-broken-super-chain` rule to account for more lifecycle hooks … #94

evanfarina commented Nov 2, 2017

rwjblue left a comment

rwjblue Nov 3, 2017

mdebbar Nov 3, 2017

evanfarina Nov 4, 2017

rwjblue Nov 3, 2017

rwjblue Nov 3, 2017

rwjblue Nov 3, 2017

evanfarina Nov 4, 2017

rwjblue Nov 3, 2017

rwjblue Nov 3, 2017

rwjblue Nov 3, 2017

mdebbar Nov 3, 2017

evanfarina Nov 4, 2017

rwjblue Nov 3, 2017

evanfarina Nov 4, 2017

mdebbar Nov 3, 2017

rwjblue Nov 4, 2017

mdebbar Nov 3, 2017

mdebbar Nov 3, 2017

mdebbar Nov 3, 2017

evanfarina Nov 4, 2017

rwjblue Nov 4, 2017

rwjblue Nov 4, 2017

evanfarina Nov 4, 2017

rwjblue Nov 4, 2017

Updated no-broken-super-chain rule to account for more lifecycle hooks … #94

Are you sure you want to change the base?

Updated no-broken-super-chain rule to account for more lifecycle hooks … #94

Conversation

evanfarina commented Nov 2, 2017

rwjblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Updated `no-broken-super-chain` rule to account for more lifecycle hooks … #94

Updated `no-broken-super-chain` rule to account for more lifecycle hooks … #94