Detect jQuery usages from ThisExpression - this.$<propertyName>

CoryDanielson commented 4 years ago

Addresses #33

Changes

These changes update the plugin to accept a boolean validateThis option that determines if rules should validate against potential jQuery usages off of this.$<propertyName>. By default, this option is false, so these changes are backwards compatible. The motivation here is to use this plugin to validate a codebase with Backbone.js, where this.$el is a standardized reference to a jQuery-wrapped element.

const myView = Backbone.View.extend({
  render() {
    this.$el.show(); // there was no warning for this before
  }
})

The only changes are that each rule accepts a config (see question below), that is passed along to isjQuery and traverse. If validateThis is enabled, traverse will return the property node rather than the root node for a MemberExpression that is a references this.<something>. This means that it would return the $el for a line that start with this.$el. The same logic is then used on this property within isjQuery to determine if it's a jQuery reference (id with $)

Tests

I've added 2 tests to each applicable rule to validate that usages of this.$foo.<disallowed-jQuery-call>() are prevented when validateThis is true, but allowed when validateThis is false. The same test code was used for the true/false conditions of validateThis, but with different options.

Open Question

This changes updates the plugin to accept the validateThis option config. Each rule accepts the option, despite many rules not using it.

Should the rules that do not use validateThis be updated to disallow potential jQuery usage off of this?
- ie: no-when does not validate for this.$.when() despite accepting the validateThis option.
Or should I remove the unused config variables from rules that do not use them?
- look for // eslint-disable-next-line no-unused-vars in my changes

Usage

// .eslintrc.js
"jquery/no-css": ["warn", {"validateThis":true}],
"jquery/no-clone": "warn",      // validateThis is false by default
"jquery/no-hide": ["warn", {"validateThis":false}],

CoryDanielson commented 4 years ago

I still need to test these changes against a repo locally to make sure that the config change is backwards compatible, and that it accepts the config as I expected it to. I copied the config code over from another popular plugin that accepts config

CoryDanielson commented 4 years ago

I just confirmed that this works locally, by importing the package from my branch/commit. I was able to update the warning level from error to warning, and the output from eslint was updated appropriately. I also added "validateThis": true and tested with it set to false and removed the property entirely and the output matched what was expected based on the unit tests.

// package.json
"eslint-plugin-jquery": "https://github.com/CoryDanielson/eslint-plugin-jquery#e0f74e319786bad8123895c9e2fb23b5d599ed6b",

edg2s commented 4 years ago

Why would you limit this to this? What if I have a closure and need to do var that = this?

Also why would this be a per-rule setting instead of a global setting?

CoryDanielson commented 4 years ago

I agree with both - I've since switched to using eslint-plugin-no-jquery. Updating this PR to use global configuration shouldn't be too much work - but it's supported out of the box in the other lint rule. I was not aware of that rule at the time of writing this.

dgraham / eslint-plugin-jquery