search

wikimedia / eslint-plugin-no-jquery

Control allowance of certain jQuery functions, and suggest or autofix alternatives.
MIT License
31 stars 8 forks source link

no-jquery/variable-pattern should not warn about the variable `this.$` #312

Closed NovemLinguae closed 1 month ago

NovemLinguae commented 3 months ago

no-jquery/variable-pattern should not warn about the variable this.$

To make dependency injection easier

class MarkFreeUseRationale {
    constructor( mw, $ ) {
        this.mw = mw;
        this.$ = $;
    }
edg2s commented 3 months ago

$/mw are always globals in the MW environment that can't be conflicted with. When does assigning to a local property help?

NovemLinguae commented 3 months ago

My idea is to use the dependency injection pattern to make the class easier to test. I can mock $ and mw and then give the mocks to the class.

edg2s commented 3 months ago

Most testing frameworks make it just as easy to mock globals (and we already do this in various repos). I don't think adding this local properties to every class is going to be worth it. There is also no limit to the number of variables that may get passed through like this eventually (window, document, other libraries).

edg2s commented 1 month ago

Closing as won't fix. If you want to do this downstream it's probably possible by modifying variablePattern, but as above I don't think we should.