Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve detection of globals and catch additional jQuery function calls in no-jquery rule #1063

Merged
merged 7 commits into from
Jan 11, 2021

Conversation

BarryThePenguin
Copy link
Contributor

@BarryThePenguin BarryThePenguin commented Dec 31, 2020

I ran into three situations when using the no-jquery rule that I thought might be worth fixing. I've added test cases for each.

  1. MemberExpression like $.extend() does not trigger the rule
  2. MemberExpression like this.$.extend() does not trigger the rule
  3. A function Parameter like function foo($) { ... } does trigger the rule

For the last one, I'd expect it to not trigger the rule, as eslint cannot know for certain where $ came from..

I put together the failing test cases and spent some time investigating what might need to change. The end result is quite a significant change to how the rule works..

All the existing test cases still work, including the new ones I've added. There may be some history behind the original implementation that I'm not aware of, so I understand if this new approach is too big a change

Fixes #211 #279

@BarryThePenguin BarryThePenguin force-pushed the fix/no-jquery-identifier branch from ad2d280 to 2e97005 Compare January 2, 2021 01:12
@bmish bmish changed the title fix: no-jquery identifier Fix some edge cases in no-jquery rule Jan 9, 2021
@bmish bmish added the bug label Jan 9, 2021
@bmish
Copy link
Member

bmish commented Jan 9, 2021

Thanks for your work so far!

@BarryThePenguin BarryThePenguin force-pushed the fix/no-jquery-identifier branch from 2e97005 to b82af74 Compare January 10, 2021 03:31
@@ -457,5 +457,21 @@ ruleTester.run('no-global-jquery', rule, {
},
],
},
{
code: `
export default Ember.Component({
Copy link
Member

@bmish bmish Jan 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to include the Ember import statement for this. Alternatively, you could just simplify the entire test case to jQuery.extend() since the rule does not actually care if the Ember component is involved and the Ember component just adds unnecessary clutter to the test case.

@bmish bmish changed the title Fix some edge cases in no-jquery rule Improve detection of globals and catch additional jQuery function calls in no-jquery rule Jan 11, 2021
Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this! This really improves and simplifies these rules.

@BarryThePenguin
Copy link
Contributor Author

Thank you! I've appreciated your help and feedback with this. I think we've ended up in a good place 👍🏻

@bmish bmish merged commit 4db2732 into ember-cli:master Jan 11, 2021
@BarryThePenguin BarryThePenguin deleted the fix/no-jquery-identifier branch January 11, 2021 03:14
@bmish
Copy link
Member

bmish commented Jan 11, 2021

@BarryThePenguin it looks like the jquery-ember-run rule could benefit from the same improvements as here, let me know if you're interested to refactor/improve that rule too. I just improved the tests for that rule (#1069) but saw many shortcomings in the rule.

@BarryThePenguin
Copy link
Contributor Author

I can take a look 👀 What shortcomings did you notice?

@bmish
Copy link
Member

bmish commented Jan 11, 2021

@BarryThePenguin the jquery-ember-run rule should be updated to use the ReferenceTracker to detect jQuery calls the same way as the no-jquery rule has been. Right now, it has very naive detection as it just checks a few string names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-global-jquery flags a pattern incorrectly
2 participants