Consider rules from eslint-plugin-jquery

[edg2s]

https://github.com/dgraham/eslint-plugin-jquery allows disabling certain jQuery functions where faster native alternatives are available.

For example we could disable $.inArray as all our compatible browsers support [].indexOf.

[edg2s]

I also would like to add performance lints, like don't use string parsing to build DOM nodes:

https://jsperf.com/jquery-buildfragment-vs-dom/1

This one is a bit more radical:

I'd like to warn against using any selectors:
$( '.dom-node-i-just-created-but-didnt-bother-to-store-in-memory' )

This is equivalent to using a global variable (e.g. window.elementsByClass['dom-node...']) and should only be used to pick up nodes written to the page by other apps.

One would have to add exceptions to a few specific init files where your app is attached to the DOM, and maybe a few inline exceptions, but it would be quite similar to our stylelint rule that bans ID selectors.

Here's an example of a bug created by such code: https://phabricator.wikimedia.org/T204749

edit: Obviously one couldn't write a regex to distinguish $( stringSelectorVariable ) from $( domNodeVariable ), so we would just limit the rule to $( 'actual strings' ) but that will still catch the vast majority of issues.

edit2: Filed as #107 & #108

[jdforrester]

I'm in favour of both of these; @jdlrobson and @Krinkle just added a series of bespoke rules to MediaWiki core's rules to effect the first: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/refs/heads/master/.eslintrc.json#14

[Krinkle]

Yeah, the no-restricted-properties rules were a good way to get started quickly, but I don't think we should use them in the long-term. At least not directly, because, while they support a custom message for display purposes, they don't have an identifier to use with eslint-disable. As such, the only way to turn them off is to turn off all rules within no-restricted-properties, which looks confusing in the code, but also means it disables more than intended.

Abstracting it within a custom rule will make the toggling code more understandable, and allows selective enabling/disabling as-needed.

The eslint-plugin-jquery is free of dependencies, well-written, well-tested, and is also being looked after well. +1 for adopting some of its rules in our preset.

Shall we compile a list here for which rules we will initially use?

[edg2s]

Well that patch seems a good place to start:

• each
• grep (not in eslint-plugin-jquery)
• inArray
• isArray (not in eslint-plugin-jquery)
• isFunction (not in eslint-plugin-jquery)
• map
• proxy
• trim

[edg2s]

Something else to consider is if we include these rules in the core config?

Most projects do use jQuery, so it would be annoying to have to add "wikimedia/jquery" to most repos. On the other hand, this is useless is most non-client JS, e.g. Node projects, and difficult to disable.

We could also move our regular rules to "wikimedia/core" and have "wikimedia" = "wikimedia/core" + "wikimedia/jquery", so Node projects could just extend "wikimedia/core".

[Krinkle]

I like the idea of splitting the base coding style from the quality rules for front-end development.

As for naming, I'd prefer to avoid "core" especially after the word "wikimedia", or anagrams of that word (^{MediaWiki core, Wikimedia Core Platform Team, Wikimedia MediaWiki Core Team, Wikimedia MediaWiki Core Team, MediaWiki CoreEvents extension , etc.}). I also think that with #97 in the pipeline, we may want to change our default away from "frontend" and give both equal presence.

E.g. extend wikimedia/frontend for front-end development, such as in OOUI, VE, and any MediaWiki-frontend integrations. And one would extend wikimedia/nodejs for Node.js services.

Given the increasing involvement of Node.js in "front-end" development, though, we'll need to decide whether we prefer build tools in front-end repositories to follow the former or the latter. Given the current differences in coding style, I would think that it be best only one of the two is involved in a given project. As such, we may want to call the services one something like wikimedia/backend or wikimedia/services.

One question, I agree it would be difficult to disable these rules if enabled by default. On the other hand, what is the use case for wanting to disable some no-jquery-x rules? If someone has a project without jQuery, the rules should be harmless and won't be mentioned anywhere in their config. If their project does use jQuery, then they probably wouldn't want to disable them all together, rather the individual rule disable method would suffice.

[jdforrester]

wikimedia/client and wikimedia/server?

[edg2s]

Given the current differences in coding style, I would think that it be best only one of the two is involved in a given project.

I do not think the "services" style should be merged here while it remains in conflict with our basic coding styles (as described on wiki), so I don't think that's going to be an issue once #97 is resolved.

If someone has a project without jQuery, the rules should be harmless and won't be mentioned anywhere in their config.

This is true at the moment, but if that ruleset grew, and other rulesets were added for other optional libraries you could end up with a very bloated ruleset.

[edg2s]

client would essentially ES5 + browser, whereas server would be ES6 + Node. These seem like sensible defaults at the moment, although we already have ES6+babel projects.

[Krinkle]

In order to scale our linting rules across the three dimensions of browser/server, es5/es6 and jquery/non-jquery - it might be best if we use mixins for additive layers, and only the base preset for ES5 vs ES6.

For example, our presets could be:

• wikimedia/base – shared base.
• wikimedia/es5 extends wikimedia/base – rules specific to ES5 parser engine, syntax and env globals.
• wikimedia/es6 extends wikimedia/base – rules specific to ES6 parser engine, syntax and env globals.

With wikimedia as alias to the current default, e.g. wikimedia/es5 (empty extend). Something we could change in the future via a major version if and when that makes sense to do.

And three mixins:

• wikimedia/qunit – rules specific to contexts with QUnit.
• wikimedia/jquery – rules specific to contexts with jQuery.
• wikimedia/node-js – rules specific to Node.js contexts.

And maybe at some point a wikimedia/browser mixin as well, although at the moment we don't (yet) have any rules specific to the browser.

Usage would be like this:

# Typical front-end project
{
  "extends": [
    "wikimedia",
    "wikimedia/jquery"
  ],
  "globals": {
    "$"

# Front-end library without jQuery
{
  "extends": [
    "wikimedia"

# Tests for typical front-end project
{
  "extends": [
    "../.eslintrc",
    "wikimedia/qunit"

# Front-end project using ES2017 via Babel and limited jQuery use
{
  "extends": [
    "wikimedia/es2017",
    "wikimedia/jquery"

# Node.js 6+ project using ES6, no jQuery
{
  "extends": [
    "wikimedia/es6",
    "wikimedia/node-js"

# Node.js 8+ project using ES2017, no jQuery
{
  "extends": [
    "wikimedia/es2017",
    "wikimedia/node-js"

[jdforrester]

Wouldn't the typical front-end project also use /es5? What'd be in es5 but not common? var stuff?

[Krinkle]

Yeah, things wikimedia/es5 not in the parent wikimedia/base, would be mainly things like var. Adding that in ES5 instead of base will make the ES6 preset cleaner by not having to undo ES5 stuff.

There would be no difference between wikimedia and wikimedia/es5, as the former would be a clean alias to the latter.

[jdforrester]

There would be no difference between wikimedia and wikimedia/es5, as the former would be a clean alias to the latter.

And then when (finally!) we move to ES6 on MW clientside, we'd move wikimedia to alias to wikimedia/es6 and clients that still wanted to work in ES5 contexts for whatever reason could manually switch to wikimedia/base plus wikimedia/es5? Works for me.

[Krinkle]

Yep, that's the idea. The default would move automatically for most projects based on the default compatibility policy (e.g. as in MediaWiki core).

For projects that directly control their own execution environment, e.g. Node.js projects supporting older versions, or standalone libraries with a different browser support matrix, they could explicitly opt to the ES-preset they want.

[edg2s]

Add global:$ to wikimedia/jquery?

[Krinkle]

@edg2s Yep, good idea. I don't recall if globals are merged by default (like rules), but assuming they are, then we can add that yeah.

[edg2s]

https://github.com/dgraham/eslint-plugin-jquery hasn't had any activity in 2 months, and there are 6 open pull requests. Perhaps we should consider forking it.

[jdlrobson]

Does this patch supersede the need for https://phabricator.wikimedia.org/T200877 ? (should I mark that as resolved?)

[jdforrester]

Does this patch supersede the need for https://phabricator.wikimedia.org/T200877 ?

Yes.

(should I mark that as resolved?)

Eh. It's fine to do so, but Phab has much higher visibility to others, so generally we keep them open until new lint rules are pushed out into at least MW. No major issue either way.

[edg2s]

Another problem upstream is that no-each doesn't distinguish between https://api.jquery.com/each/ and http://api.jquery.com/jquery.each/ , i.e. $( selector ).each( fn ) vs $.each( array, fn )

[edg2s]

We forked the library due to a number of outstanding issues: https://github.com/wikimedia/eslint-plugin-jquery, including those mentioned above, and because the maintainer appears to be inactive.