[NEW RULE] avoid-leaking-callbacks-in-ember-objects #422

rajasegar · 2019-05-25T01:39:47Z

Fixes #423
Added new rule and tests to avoid callback memory leaks
Callback leaks are memory leaks that occur due to state being caught in a callback function that is never released from memory.

Read more on callback leaks here

Added new rule and tests to avoid callback memory leaks

lib/rules/no-callback-leaks-in-ember-objects.js

bmish · 2019-06-03T02:15:45Z

lib/rules/no-callback-leaks-in-ember-objects.js

+    const addedListeners = [];
+    const removedListeners = [];
+    return {
+      'ExportDefaultDeclaration:exit': function (node) {


Is ExportDefaultDeclaration:exit the best time to run the leak checks? If there are multiple Ember objects in a file, including some after the default export, then running the leak checks at this point wouldn't be sufficient, right?

I believe Program:exit would be better since this runs when we're done visiting the entire file.

Changed to Program:exit

bmish · 2019-06-03T02:16:35Z

lib/rules/no-callback-leaks-in-ember-objects.js

+
+      CallExpression(node) {
+        // console.log(node.callee.property);
+        if (utils.getPropertyValue(node, 'callee.property.name') === 'addEventListener') {


It would be preferred to use native JavaScript (node.callee.property.name) to access this data instead of using the helper function.

For some reasons, it is not working, through the utils method it was working fine thought, that's why i had that console.log previously, now removed it

bmish · 2019-06-03T02:19:38Z

lib/rules/no-callback-leaks-in-ember-objects.js

+
+  create(context) {
+    const report = function (node) {
+      const message = "Event listeners and interval timers must be unregistered or ensure that the context they're registered with is destroyed, when no longer needed.";


The following error message would be clearer:

All `addEventListener` calls should have a corresponding `removeEventListener` to avoid leaking callbacks.

Message changed

bmish · 2019-06-03T02:20:56Z

README.md

@@ -129,6 +129,7 @@ The `--fix` option on the command line automatically fixes problems reported by
 |:---|:--------|:------------|
 | :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 |
+| :white_check_mark: | [no-callback-leaks-in-ember-objects](./docs/rules/no-callback-leaks-in-ember-objects.md) | Avoids callback memory leaks |


Should we name this rule avoid-leaking-callbacks-in-ember-objects for consistency with the existing avoid-leaking-state-in-ember-objects rule name?

Yep, makes sense for me too.

bmish · 2019-06-03T02:34:23Z

README.md

@@ -129,6 +129,7 @@ The `--fix` option on the command line automatically fixes problems reported by
 |:---|:--------|:------------|
 | :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 |
+| :white_check_mark: | [no-callback-leaks-in-ember-objects](./docs/rules/no-callback-leaks-in-ember-objects.md) | Avoids callback memory leaks |


Remember to run yarn update and add the rule to index.js too.

bmish · 2019-06-03T02:35:27Z

lib/rules/no-callback-leaks-in-ember-objects.js

+      url: 'https://github.com/ember-best-practices/memory-leak-examples/blob/master/exercises/exercise-2.md'
+    },
+    fixable: null, // or "code" or "whitespace"
+    schema: [{


schema isn't being used so you should remove this.

schema removed

bmish · 2019-06-03T02:39:10Z

tests/lib/rules/no-callback-leaks-in-ember-objects.js

+});`,
+    },
+  ],
+  invalid: [


I can't really tell the difference between most of these invalid test cases.

It's not worth having separate test cases just both Ember.Component.extend and Component.extend.

I would get rid of the duplicate test cases and keep only these test cases:

One callback which gets leaked

Two callbacks which get leaked

One callback which gets leaked and one callback which doesn't get leaked

If there are more legitimate test cases, definitely include them, but it would be helpful to add a comment explaining what each test case is testing.

We are now having 3 invalid cases as per you mentioned, will add more if possible later

bmish · 2019-06-03T02:40:32Z

tests/lib/rules/no-callback-leaks-in-ember-objects.js

+// ------------------------------------------------------------------------------
+
+
+const message = "Event listeners and interval timers must be unregistered or ensure that the context they're registered with is destroyed, when no longer needed.";


This error message should be imported directly from the rule file instead of copied here. See the no-test-and-then rule for an example.

bmish · 2019-06-03T02:44:59Z

lib/rules/no-callback-leaks-in-ember-objects.js

+
+          if (idx < 0) {
+            // No removeEventListener
+            report(node);


node is not the right element to report the violation on. You should report the violation on the specific addEventListener statement node.

Now, i am adding the node of the addEventListener object itself, so that we will get the right location for the error

1. Moving error messages to exports 2. Removing commented code 3. Changing the erro message to new string 4. Ran yarn update

1. Renaming the rule to `avoid-leaking-callbacks-in-ember-objects`

1. Updating rule names in Readme 2. Adding rule to index.js 3. Adding node information for proper error reporting by using addEventListener nodes 4. Fixing test cases and removing duplicates

rajasegar · 2019-06-03T08:56:18Z

@bmish Thanks for reviewing, addressed the comments in the latest changes

Ran yarn update to update the new rule names in readme and recommended rules

bmish · 2019-06-03T20:11:07Z

lib/index.js

@@ -47,6 +47,7 @@ module.exports = {
    'routes-segments-snake-case': require('./rules/routes-segments-snake-case'),
    'use-brace-expansion': require('./rules/use-brace-expansion'),
    'use-ember-get-and-set': require('./rules/use-ember-get-and-set'),
+    'avoid-leaking-callbacks-in-ember-objects': require('./rules/avoid-leaking-callbacks-in-ember-objects'),


This should be alphabetical in the list.

Moved to the top in alphabetical order

bmish · 2019-06-03T20:11:38Z

README.md

@@ -104,6 +104,15 @@ The `--fix` option on the command line automatically fixes problems reported by
 | :wrench: | [use-ember-get-and-set](./docs/rules/use-ember-get-and-set.md) | Enforces usage of Ember.get and Ember.set |


+### Ember Object


I don't think this section was supposed to move. yarn update should generate this file for you.

I think after running yarn update only moved this block, i don't remember manually doing it. Anyway restored the position for now

bmish · 2019-06-03T20:14:02Z

tests/__snapshots__/recommended.js.snap

@@ -22,5 +22,6 @@ Array [
  "require-super-in-init",
  "routes-segments-snake-case",
  "use-brace-expansion",
+  "avoid-leaking-callbacks-in-ember-objects",


Remove this change.

bmish · 2019-06-03T20:15:20Z

tests/lib/rules/avoid-leaking-callbacks-in-ember-objects.js

+  valid: [
+    {
+      code: `
+export default Ember.Component.extend({


This test case seems to duplicate the other valid test case. Let's keep the Component.extend but not the older syntax Ember.Component.extend one.

Duplicate case removed.

bmish · 2019-06-03T20:16:24Z

lib/rules/avoid-leaking-callbacks-in-ember-objects.js

+    docs: {
+      description: 'Avoids callback memory leaks',
+      category: 'Ember Object',
+      recommended: true,


Rules should never be added as recommended since that would be a breaking change. We can consider making it recommended in the future.

Removed the recommended flag

bmish · 2019-06-03T20:17:06Z

lib/rules/avoid-leaking-callbacks-in-ember-objects.js

+
+const ERROR_MESSAGE = 'All `addEventListener` calls should have a corresponding `removeEventListener` to avoid leaking callbacks.';
+//------------------------------------------------------------------------------
+// Ember object rule - Avoid callback memory leaks


Should this rule be limited to Ember objects only?

If it should, it would need a if (!(ember.isEmberObject(node) || ember.isReopenObject(node))) check somewhere, similar to the existing avoid-leaking-state-in-ember-objects rule.

If it's not necessary to limit it to Ember objects, and we can just run it on a per-file basis, then we would want to rename the rule to avoid mentioning in-ember-objects, and remove all the extra Ember object code from the test cases.

I am afraid we can't directly use the if (!(ember.isEmberObject(node) || ember.isReopenObject(node))) checks here since we are actually targeting the addEventListener and removeEventListener call expressions we can't check them using the above utils.

And if we are not limiting this to Ember objects, is it OK to rename the rule to `avoid-leaking-callbacks'

Is there a reason why this rule is specific to Ember?

If this rule is not specific to Ember, then it could just be a generic JavaScript eslint rule, and likely would not belong in eslint-plugin-ember. (I found a similar rule here)

If this rule is specific to Ember, then we need to update the code to actually limit it to something like Ember objects/components. In this case, you could write a helper function like isInsideEmberObject(node) which loops/recurses upwards in the tree checking each ancestor.

Interested to get the opinion of @Turbo87 @rwjblue on this.

@bmish I tend to agree. I don't see a reason to scope this to just Ember objects.

@rajasegar I wonder if you could try contributing this rule to eslint itself as a general no-leaking-callbacks rule? They are pretty strict about accepting new rules but perhaps you could at least ask what they think about it since it seems like a general JavaScript rule.

Sure @bmish I will try

1. Restoing Ember Object section in Readme 2. Rearranging the new rule in lib/index based on alphabetical order 3. Removing commented code in the rule 4. Removing the recommended flag from the rule 5. Removing the rule from snapshot recommended 6. Removing the duplicate valid test case with Ember.Component.extend

rajasegar · 2019-06-04T07:26:08Z

@bmish Addressed the review comments, please check and let me know

bmish · 2019-06-04T16:25:59Z

lib/rules/avoid-leaking-callbacks-in-ember-objects.js

+
+const ERROR_MESSAGE = 'All `addEventListener` calls should have a corresponding `removeEventListener` to avoid leaking callbacks.';
+//------------------------------------------------------------------------------
+// Ember object rule - Avoid callback memory leaks


Is there a reason why this rule is specific to Ember?

If this rule is not specific to Ember, then it could just be a generic JavaScript eslint rule, and likely would not belong in eslint-plugin-ember. (I found a similar rule here)

If this rule is specific to Ember, then we need to update the code to actually limit it to something like Ember objects/components. In this case, you could write a helper function like isInsideEmberObject(node) which loops/recurses upwards in the tree checking each ancestor.

Interested to get the opinion of @Turbo87 @rwjblue on this.

bmish · 2019-06-05T05:27:58Z

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-callbacks-in-ember-objects](./docs/rules/avoid-leaking-callbacks-in-ember-objects.md) | Avoids callback memory leaks |


:white_check_mark: is only supposed to be present for recommended rules. If you run yarn update, this doc would be automatically generated without this since it's not longer a recommended rule.

1. Removing white checkmark for no callback leaks rules in README

Rajasegar and others added 3 commits May 25, 2019 07:07

[NEW RULE] no-callback-leaks

8012125

Added new rule and tests to avoid callback memory leaks

[DOCS] Adding doc page for the rule and ref in README

707e9a8

Merge branch 'master' into avoid-callback-leaks

adfa4eb

bmish requested changes Jun 3, 2019

View reviewed changes

rajasegar added 3 commits June 3, 2019 11:43

[CHORE] Refactoring the rule and tests based on review comments

87bed74

1. Moving error messages to exports 2. Removing commented code 3. Changing the erro message to new string 4. Ran yarn update

[CHORE] Renaming rules and assets

4fb3db2

1. Renaming the rule to `avoid-leaking-callbacks-in-ember-objects`

[CHORE] Fixing rule names in all the places

4cabe4c

1. Updating rule names in Readme 2. Adding rule to index.js 3. Adding node information for proper error reporting by using addEventListener nodes 4. Fixing test cases and removing duplicates

rajasegar added 2 commits June 3, 2019 14:27

[CHORE] Updated rule name in the docs

3d4cf72

[CHORE] Yarn update task for callback leaks rule

db6d2df

Ran yarn update to update the new rule names in readme and recommended rules

bmish changed the title ~~[NEW RULE] no-callback-leaks~~ [NEW RULE] avoid-leaking-callbacks-in-ember-objects Jun 3, 2019

bmish requested changes Jun 3, 2019

View reviewed changes

bmish requested changes Jun 5, 2019

View reviewed changes

[CHORE] Review comments - phase 3

eb30a54

1. Removing white checkmark for no callback leaks rules in README

		// ------------------------------------------------------------------------------


		const message = "Event listeners and interval timers must be unregistered or ensure that the context they're registered with is destroyed, when no longer needed.";

		@@ -104,6 +104,15 @@ The `--fix` option on the command line automatically fixes problems reported by
		\| :wrench: \| [use-ember-get-and-set](./docs/rules/use-ember-get-and-set.md) \| Enforces usage of Ember.get and Ember.set \|


		### Ember Object

[NEW RULE] avoid-leaking-callbacks-in-ember-objects #422

Are you sure you want to change the base?

[NEW RULE] avoid-leaking-callbacks-in-ember-objects #422

Conversation

rajasegar commented May 25, 2019 • edited Loading

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

rajasegar commented Jun 3, 2019

Choose a reason for hiding this comment

rajasegar Jun 4, 2019 • edited Loading

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

rajasegar Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

bmish Jun 4, 2019 • edited Loading

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

rajasegar commented Jun 4, 2019

bmish Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajasegar commented May 25, 2019 •

edited

Loading

rajasegar Jun 4, 2019 •

edited

Loading

rajasegar Jun 4, 2019 •

edited

Loading

bmish Jun 4, 2019 •

edited

Loading

bmish Jun 4, 2019 •

edited

Loading