Add rule to disallow `this.$` to prepare apps to remove jQuery #121

cibernox · 2017-08-04T18:26:30Z

In modern browsers jQuery is less and less needed as many features are not built in
into modern browsers and the bugs that jQuery used to help you avoid have been fixed.
However, jQuery still adds ~30KB (min + gzip) to every Ember app, which can add up to
1 second of download + parse times on mobile devices. Is is now possible to remove
jQuery from Ember apps, and one of the main features in Ember that cannot be used on
an app that intends to remove jQuery in a near future is this.$, either inside
component's methods or within tests.

This PR adds a rule, that is entirely optional and I do not think it should
be enabled by default to yell at anyone who uses this.$.

Please let me know what else has to be done (documentation, etc...) to make
this mergeable.

kevinkucharczyk · 2017-08-07T08:03:07Z

Thank you for the PR @cibernox! Personally I think this is a fantastic idea and definitely a step in the right direction. I also agree on not making the rule enabled by default, since it would break a lot of apps.
You should add a documentation file for this rule under docs/rules and finally run yarn run update to automatically update the repo's readme with your new rule. There's a short contribution guide in the readme here: https://github.com/ember-cli/eslint-plugin-ember#-contribution-guide

cibernox · 2017-08-07T08:46:42Z

README.md

+
+|    | Rule ID | Description |
+|:---|:--------|:------------|
+|  | [no-this-jquery](./docs/rules/no-this-jquery.md) | Disallow use of `this.# eslint-plugin-ember


Seems that the script doesn't like this.$ 🤔

cibernox · 2017-08-07T08:47:22Z

I added docs and regenerated the readme, but I think there is too many changes.

cibernox · 2017-08-07T23:32:44Z

After some conversation, this rule is going to be edited to be no-jquery and y must forbid several things.

Tasks:

cibernox · 2017-08-08T09:23:43Z

I've updated the PR to be a no-jquery, which covers pretty much any reasonable usage of jQuery.

rwjblue · 2017-08-09T13:11:43Z

README.md

+
+|    | Rule ID | Description |
+|:---|:--------|:------------|
+|  | [no-jquery](./docs/rules/no-jquery.md) | Disallow use of `this.# eslint-plugin-ember


this.# should be this.$ (and there is a missing backtick)

I know, but this is autogenerated. If we don't fix the error in the generator the next one in running the script will ruin this again.

Gotcha! I didn't realize it was auto-generated, sorry 😸

We should make sure to create an issue for that (so we can track down the bug).

lynchbomb · 2017-08-09T14:21:29Z

Have a look at https://github.com/ember-best-practices/eslint-plugin-ember-best-practices/blob/master/lib/rules/no-jquery-methods.js might help cover any corner-cases you run into.

cibernox · 2017-08-09T14:22:39Z

@lynchbomb I believe that I've covered all that. Do you see anything not covered?

michalsnik

Please set additionally in no-global-jquery's meta docs:

replacedBy: 'no-jquery',
deprecated: true

I'll add extra list with replaced rules in readme later.

michalsnik · 2017-08-10T14:40:09Z

lib/rules/no-jquery.js

+module.exports = {
+  meta: {
+    docs: {
+      description: 'Disallow use of `this.$` on components or tests',


You should update description of the rule, as now it disallows any jQuery usage. When you'll do this, please revert readme and execute npm run update once again.

michalsnik · 2017-08-10T14:46:10Z

lib/rules/no-jquery.js

+      },
+
+      MemberExpression(node) {
+        if ((node.object.name === 'Ember' || node.object.name === 'Em' || (emberImportAliasName && node.object.name === emberImportAliasName)) && node.property.name === '$') {


Please wrap this line to make it more readable

michalsnik · 2017-08-10T14:48:03Z

lib/rules/no-jquery.js

+          context.report(node, message);
+        }
+
+        if (isThisJquery(node)) {


I'm fine with this, however just to let you know you could also use eslint selectors:

'CallExpression > MemberExpression > ThisExpression' (node) { if (node.parent.property.name === '$') { // busted } }

TIL. I'm pretty new to ESLint rules, but this is neat 👌

michalsnik · 2017-08-10T14:49:18Z

lib/rules/no-jquery.js

+
+      MemberExpression(node) {
+        if ((node.object.name === 'Ember' || node.object.name === 'Em' || (emberImportAliasName && node.object.name === emberImportAliasName)) && node.property.name === '$') {
+          report(node, message);


This function doesn't expect second argument. Anyway you should either use report function everywhere or context.report

michalsnik · 2017-08-10T14:51:01Z

lib/rules/no-jquery.js

+        }
+
+        if (isThisJquery(node)) {
+          report(node, specialisedMessageForThisJquery);


As I wrote above report doesn't seem to accept the second argument.

michalsnik · 2017-08-10T14:53:10Z

tests/lib/rules/no-jquery.js

+      }]
+    },
+    {
+      code: `


Just to make this test case complete, please add filename field with real path including tests/

cibernox · 2017-08-11T12:34:10Z

@michalsnik I've applied the feedback

scalvert · 2017-08-13T01:17:21Z

This seems good if we're extending no-global-jquery. I'm all for consolidating in this rule.

The intent of no-global-jquery was to ensure that people were using Ember.$|Ember.jQuery vs. global $|jQuery. IMO the rules are different, even though they overlap.

Perhaps we should instead keep them distinct?

michalsnik · 2017-08-15T12:52:13Z

Having two rules that overlaps might be a bit strange @scalvert. On the other hand that could be intentional. Perhaps if we recommend one rule over another in recommended configuration they could live alongside each other 🤔 WDYT @cibernox @Turbo87 ? I'm ok with both approaches to be honest..

cibernox · 2017-08-15T19:44:37Z

@michalsnik As part of this PR, requested on the code review, I've marked the no-global-jquery rule as deprecated and replaced by this one, so your suggestion is already done.

I wouldn't oppose to allow both to coexist. This one is a superset of the other one, so there is no point in enabling the other one if this one is, but I think it's just a matter of documenting in the readme how both rules are different and people can decide which one they want to enable.

michalsnik · 2017-08-15T20:10:48Z

Yeah that was my recommendation if we were to remove the other rule in the next major release. But if we decide to leave it than my recommendation was wrong and we should keep both rules, but only of them marked as recommended - the new one. But given the fact that any change to recommended setting is a breaking change we would change our recommendations in the next major release (+/- 2 weeks) and for now keep it as is (recommended: false). That would also give us enough time to see how the new rule works in real projects.

Sorry for the confusion @cibernox. But I thought this through once more after last comment from @scalvert and I'd remove:

replacedBy: 'no-jquery',
deprecated: true

which I proposed before. We're probably not going to deprecate this rule, as in old living projects removing jQuery could be a truly daunting task, but making sure that it doesn't use global jquery would be a quick win, small step towards better code base. With this in mind removing no-global-jquery would not be so smart move.

Turbo87 · 2017-08-16T06:28:02Z

@michalsnik again I strongly advise against yet another major release in the near future. It will create a lot of unnecessary churn and IMHO we should wait a few months until things have settled down a bit.

In modern browsers jQuery is less and less needed as many features are not built in into modern browsers and the bugs that jQuery used to help you avoid have been fixed. However, jQuery still adds ~30KB (min + gzip) to every Ember app, which can add up to 1 second of download + parse times on mobile devices. Is is not possible to remove jQuery from Ember apps, and one of the main features in Ember that cannot be used on an app that intends to remove jQuery in a near future is `this.$`, either inside component's methods or within tests. This PR adds a rule, that is entirely options and **I do not think** it should be enabled by default to yell at anyone who uses `this.$`. Please let me know what else has to be done (documentation, etc...) to make this mergeable.

cibernox · 2017-08-16T08:30:39Z

@michalsnik I've removed those options so both rules coexist. Also since I've changed the description of the rule, #127 doesn't affect the new docs.

michalsnik

LGTM 👍

cibernox · 2017-08-17T15:33:43Z

Can we merge?

michalsnik · 2017-09-01T23:23:32Z

Thanks @cibernox ! Released in v4.5.0 🎉

cibernox commented Aug 7, 2017

View reviewed changes

cibernox mentioned this pull request Aug 7, 2017

Rule to disallow jQuery #123

Closed

cibernox force-pushed the disallow-this-jquery branch from 8baabdd to d875eb5 Compare August 8, 2017 09:19

rwjblue reviewed Aug 9, 2017

View reviewed changes

cibernox mentioned this pull request Aug 9, 2017

Documentation task has problems with the $ sign #127

Closed

michalsnik suggested changes Aug 10, 2017

View reviewed changes

cibernox added 5 commits August 16, 2017 10:25

Add docs

a3f1b2a

Preparing for a new no-jquery rule. Added failing tests.

4fb01e6

Cover all reasonable usages of jQuery

cc6103b

Apply feedback

79fef77

cibernox force-pushed the disallow-this-jquery branch from aa988fe to 419cd50 Compare August 16, 2017 08:26

Remove deprecation informtion

d55c791

cibernox force-pushed the disallow-this-jquery branch from 419cd50 to d55c791 Compare August 16, 2017 08:28

michalsnik approved these changes Aug 17, 2017

View reviewed changes

michalsnik merged commit 4dcc3f0 into ember-cli:master Sep 1, 2017

Turbo87 added the enhancement label Nov 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rule to disallow `this.$` to prepare apps to remove jQuery #121

Add rule to disallow `this.$` to prepare apps to remove jQuery #121

cibernox commented Aug 4, 2017 •

edited

Loading

kevinkucharczyk commented Aug 7, 2017

cibernox Aug 7, 2017

cibernox commented Aug 7, 2017

cibernox commented Aug 7, 2017 •

edited

Loading

cibernox commented Aug 8, 2017

rwjblue Aug 9, 2017

cibernox Aug 9, 2017

rwjblue Aug 9, 2017

rwjblue Aug 9, 2017

cibernox Aug 9, 2017

scalvert Aug 13, 2017

lynchbomb commented Aug 9, 2017

cibernox commented Aug 9, 2017

michalsnik left a comment

michalsnik Aug 10, 2017

michalsnik Aug 10, 2017

michalsnik Aug 10, 2017

cibernox Aug 11, 2017

michalsnik Aug 10, 2017

michalsnik Aug 10, 2017

michalsnik Aug 10, 2017

cibernox commented Aug 11, 2017

scalvert commented Aug 13, 2017

michalsnik commented Aug 15, 2017

cibernox commented Aug 15, 2017

michalsnik commented Aug 15, 2017

Turbo87 commented Aug 16, 2017

cibernox commented Aug 16, 2017

michalsnik left a comment

cibernox commented Aug 17, 2017

michalsnik commented Sep 1, 2017

Add rule to disallow this.$ to prepare apps to remove jQuery #121

Add rule to disallow this.$ to prepare apps to remove jQuery #121

Conversation

cibernox commented Aug 4, 2017 • edited Loading

kevinkucharczyk commented Aug 7, 2017

Choose a reason for hiding this comment

cibernox commented Aug 7, 2017

cibernox commented Aug 7, 2017 • edited Loading

cibernox commented Aug 8, 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

lynchbomb commented Aug 9, 2017

cibernox commented Aug 9, 2017

michalsnik 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

cibernox commented Aug 11, 2017

scalvert commented Aug 13, 2017

michalsnik commented Aug 15, 2017

cibernox commented Aug 15, 2017

michalsnik commented Aug 15, 2017

Turbo87 commented Aug 16, 2017

cibernox commented Aug 16, 2017

michalsnik left a comment

Choose a reason for hiding this comment

cibernox commented Aug 17, 2017

michalsnik commented Sep 1, 2017

Add rule to disallow `this.$` to prepare apps to remove jQuery #121

Add rule to disallow `this.$` to prepare apps to remove jQuery #121

cibernox commented Aug 4, 2017 •

edited

Loading

cibernox commented Aug 7, 2017 •

edited

Loading