Adding new rule: require-ember-lifeline #62

scalvert · 2017-06-22T21:24:00Z

This rule identifies the use of Ember.run.* and indicates the requirement to use runTask, debounceTask, or throttleTask from ember-lifeline in favor of the run variant.

trentmwillis

Do we expect there to be anything else we lint for with ember-lifeline? If so, we might want a more specific rule name.

trentmwillis · 2017-06-23T02:40:17Z

guides/rules/require-ember-lifeline.md

+
+Ember applications have long life-cycles. A user may navigate to several pages and use many different features before they leave the application. This makes JavaScript and Ember development unlike Rails development, where the lifecycle of a request is short and the environment disposed of after each request. It makes Ember development much more like iOS or video game development than traditional server-side web development.
+
+It is good to note that this isn't something inherent to just Ember. Any single-page app framework or solution (Angular, React, Vue, Backbone...) must deal this lifecycles of objects, and specifically with how async tasks can be bounded by a lifecycle.


Nit: "must deal this lifecycles" -> "must deal with the lifecycles"

trentmwillis · 2017-06-23T02:43:35Z

lib/rules/require-ember-lifeline.js

+        }
+      },
+
+      ImportDefaultSpecifier(node) {


Nit: Can we move this above ObjectPattern? Since it is expected to run prior to that function.

trentmwillis · 2017-06-23T02:46:19Z

lib/rules/require-ember-lifeline.js

+    return {
+      ObjectPattern(node) {
+        if (emberImportBinding) {
+          LATER_PATTERNS = LATER_PATTERNS.concat(collectObjectPatternBindings(node, {


If I understand this correctly, this code catches if run.later was bound to something else, correct? If so, can we add a test for this scenario?

Yep, on it!

trentmwillis · 2017-06-23T18:10:27Z

lib/rules/require-ember-lifeline.js

@@ -0,0 +1,48 @@
+/**
+ * @fileOverview Require the use of ember-lifeline's runTask over using Ember.run.later.
+ * @author Steve Calvert


After spending a bit of time thinking about it, I'd like for us to stop placing @author annotations in these files. They feel territorial and might off put newer contributors.

If we want to acknowledge authors/contributors, we should go the route of placing an AUTHORS.txt file in the root of the repo which can be generated from the git history (example from QUnit).

Totally agree. AUTHORS.txt works for me.

But how will the world know about my amazing feats of coding?

Totally agree. I'll remove from all rules in a separate PR.

This is done.

lynchbomb · 2017-06-23T18:32:25Z

lib/rules/require-ember-lifeline.js

@@ -0,0 +1,48 @@
+/**
+ * @fileOverview Require the use of ember-lifeline's runTask over using Ember.run.later.
+ * @author Steve Calvert


Totally agree. AUTHORS.txt works for me.

lynchbomb · 2017-06-23T18:34:17Z

lib/rules/require-ember-lifeline.js

+
+module.exports = {
+  docs: {
+    description: 'Please use runTask from ember-lifeline in favor of Ember.run.later.',


Can you leverage the MESSAGE variable rather than duplicating this message?

As you can see now, the message is dynamically generated based on the node being inspected. I've adjusted it now.

lynchbomb · 2017-06-23T18:38:44Z

tests/lib/rules/require-ember-lifeline.js

+            });
+          }
+        });`,
+      parserOptions: {


Can we move the parserOptions up into the RuleTester() and then remove the duplicated parserOptions?

example:

const ruleTester = new RuleTester({ parserOptions: { ... } });

scalvert · 2017-06-23T22:58:51Z

@trentmwillis updated to include both debounce and throttle. Can you take another look?

scalvert · 2017-06-26T14:37:08Z

Bump @trentmwillis

lynchbomb · 2017-06-26T15:39:12Z

tests/lib/rules/require-ember-lifeline.js

-        ecmaVersion: 6,
-        sourceType: 'module'
-      }
+      parserOptions


Can we delete these redundant parserOptions?

Same as above.

lynchbomb · 2017-06-26T15:40:55Z

tests/lib/rules/require-ember-lifeline.js

@@ -1,26 +1,31 @@
 const rule = require('../../../lib/rules/require-ember-lifeline');
-const MESSAGE = rule.meta.message;
+const getMessage = rule.meta.message;
 const RuleTester = require('eslint').RuleTester;
 const ruleTester = new RuleTester();


Can we just pass the parserOptions object directly into the instantiation of RuleTester(). That way we can delete the redundant parserOptions from each assertion.

We can, but do we care? If we do, we should do this across all the test files. That should be done separately in a different PR. Thoughts?

IDK I think it's more than a nit. We should prob make this change to all the tests. Figure it's still early days within this repo and we can solidify the ideals.

Essentially we have lines of code within tests that are redundant. I'm all for a balance of succinct yet grokkable. Thoughts?

I'm happy to refactor this, just want it to be in a separate PR. I created #65 to address this.

rwjblue · 2017-06-26T21:52:38Z

lib/rules/require-ember-lifeline.js

+
+    return {
+      ImportDefaultSpecifier(node) {
+        emberImportBinding = getEmberImportBinding(node);


We should also handle:

import run from '@ember/runloop'; import { debounce } from '@ember/runloop'; import { throttle } from '@ember/runloop'; import { later } from '@ember/runloop'; import { next } from '@ember/runloop';

Could be done in a follow up PR though...

Sounds good.

* Adding new rule: require-ember-lifeline

Adding new rule: require-ember-lifeline

bf027df

scalvert requested review from stefanpenner, rwjblue, chadhietala, runspired, trentmwillis and lynchbomb June 22, 2017 21:24

trentmwillis requested changes Jun 23, 2017

View reviewed changes

trentmwillis reviewed Jun 23, 2017

View reviewed changes

lynchbomb requested changes Jun 23, 2017

View reviewed changes

scalvert added 2 commits June 23, 2017 15:52

Adding extra support for Ember.run.debounce and Ember.run.throttle

d01f47d

Fixing parserOptions in tests

8404ce9

scalvert added 3 commits June 23, 2017 16:05

Updating param name for clarity

96d5b8f

Updating description

f20ab3b

Removing author from file

d1a42cd

lynchbomb reviewed Jun 26, 2017

View reviewed changes

Adding support for checking for next too.

233596b

rwjblue approved these changes Jun 26, 2017

View reviewed changes

scalvert merged commit 7604b1e into master Jun 26, 2017

scalvert deleted the require-ember-lifeline branch June 26, 2017 22:03

lynchbomb pushed a commit to lynchbomb/eslint-plugin-ember-best-practices that referenced this pull request Jul 26, 2017

Adding new rule: require-ember-lifeline (ember-best-practices#62)

02167a5

* Adding new rule: require-ember-lifeline

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding new rule: require-ember-lifeline #62

Adding new rule: require-ember-lifeline #62

scalvert commented Jun 22, 2017 •

edited

Loading

trentmwillis left a comment

trentmwillis Jun 23, 2017

scalvert Jun 23, 2017

trentmwillis Jun 23, 2017

scalvert Jun 23, 2017

scalvert Jun 26, 2017

trentmwillis Jun 23, 2017

scalvert Jun 23, 2017

scalvert Jun 26, 2017

trentmwillis Jun 23, 2017

lynchbomb Jun 23, 2017

scalvert Jun 24, 2017

scalvert Jun 26, 2017

lynchbomb Jun 23, 2017

lynchbomb Jun 23, 2017 •

edited

Loading

scalvert Jun 24, 2017

lynchbomb Jun 23, 2017

scalvert Jun 23, 2017

scalvert Jun 26, 2017

scalvert commented Jun 23, 2017

scalvert commented Jun 26, 2017

lynchbomb Jun 26, 2017

scalvert Jun 26, 2017

lynchbomb Jun 26, 2017

scalvert Jun 26, 2017

lynchbomb Jun 26, 2017

scalvert Jun 26, 2017

rwjblue Jun 26, 2017

scalvert Jun 26, 2017

scalvert Jun 26, 2017


		Ember applications have long life-cycles. A user may navigate to several pages and use many different features before they leave the application. This makes JavaScript and Ember development unlike Rails development, where the lifecycle of a request is short and the environment disposed of after each request. It makes Ember development much more like iOS or video game development than traditional server-side web development.

		It is good to note that this isn't something inherent to just Ember. Any single-page app framework or solution (Angular, React, Vue, Backbone...) must deal this lifecycles of objects, and specifically with how async tasks can be bounded by a lifecycle.

Adding new rule: require-ember-lifeline #62

Adding new rule: require-ember-lifeline #62

Conversation

scalvert commented Jun 22, 2017 • edited Loading

trentmwillis 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

lynchbomb Jun 23, 2017 • 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

scalvert commented Jun 23, 2017

scalvert commented Jun 26, 2017

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

scalvert commented Jun 22, 2017 •

edited

Loading

lynchbomb Jun 23, 2017 •

edited

Loading