Page MenuHomePhabricator

Forbid use of $.each, $.map, $.isArray in core JavaScript code
Closed, ResolvedPublic8 Story Points

Description

Various jQuery methods have native equivalents and are supported by all grade A browsers in MediaWiki, yet in core we continue to use jQuery methods

We should modernize this code and enforce usage of native JavaScript via eslint going forward.

Acceptance criteria

For purpose of Google-Code-In this can be separated into tasks if necessary.

Event Timeline

Jdlrobson created this task.Aug 1 2018, 7:26 AM
Restricted Application added a project: Performance-Team. · View Herald TranscriptAug 1 2018, 7:26 AM

Change 449670 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Warn against certain jQuery methods

https://gerrit.wikimedia.org/r/449670

Schnark added a subscriber: Schnark.Aug 1 2018, 7:51 AM

$.each and $.map can be used on plain objects as well (http://api.jquery.com/jquery.each/#jQuery-each-object-callback, http://api.jquery.com/jquery.map/#jQuery-map-object-callback), where they doesn't have simple equivalents in core JS. $.map is a bit strange on objects, as it will return an array (which may be unexpected), so it might be a good idea to avoid it anyway, but I think $.each( object, callback ) is a valid use that shouldn't be deprecated.

but I think $.each( object, callback ) is a valid use that shouldn't be deprecated.

Why? What is the advantage of using this over casting to an array and using Array.forEach ?

To be clear $('.foo').each would not be deprecated and $.each could still be used using an eslint-ignore comment
e.g.

/* eslint-disable-line  no-restricted-properties */

but I think $.each( object, callback ) is a valid use that shouldn't be deprecated.

Why? What is the advantage of using this over casting to an array and using Array.forEach ?

I do agree with you that $.each shouldn't be used for arrays, where forEach is the better choice, but that doesn't work on plain objects.

Example:

$.each( {
  foo: 1,
  bar: 2
}, function ( key, value ) {
  console.log( 'Key:', key, 'Value:', value );
} );

That can't be easily replicated with core JS.

Any reason not to use Object.keys here?

var items = {
  foo: 1,
  bar: 2
};
Object.keys( items ).forEach(function (key) {
console.log( 'Key:',key, 'Value:', items[key] );  
})

Also more longer term we'll be able to do:

Object.entries( {
  foo: 1,
  bar: 2
} ).forEach(function (item) {console.log( 'Key:', item[0], 'Value:', item[1] );  } )

Your first approach does work, but can require some re-writing (especially as you have to assign the object to a variable if it isn't already), which probably is acceptable, but it definitely isn't a trivial replacement like the others.
Your second approach doesn't work, due to limited browser support (i.e., it doesn't work in all Grade A browsers, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries#Browser_compatibility).

That's why i said more longer term :)

Note first example can also make use of this rather than casting to a variable.

Niedzielski changed the edit policy from "Custom Policy" to "All Users".Aug 1 2018, 4:31 PM
ovasileva added a subscriber: ovasileva.

Discussed in grooming today and there was some confusion around whether readers web was responsible for making these changes. Moving to needs analysis.

Any reason not to use Object.keys here?

var items = {
  foo: 1,
  bar: 2
};
Object.keys( items ).forEach( function ( key ) {
  console.log( 'Key:',key, 'Value:', items[ key ] );  
})

For objects, it seems like a for-loop would be simpler (and inherently slightly faster):

var items = {
  foo: 1,
  bar: 2
};
for ( key in items ) {
  console.log( 'Key:', key, 'Value:', items[ key ] );  
}
Jdlrobson added a project: MediaWiki-Interface.

Discussed in grooming today and there was some confusion around whether readers web was responsible for making these changes. Moving to needs analysis.

Task is ready and can be estimated. It should be straightforward and clear please let me know if it is not. I don't think this column makes sense for ownership discussions (consider priority low/tracking/product owner backlog instead)

With regards to this question, as consumers of the core shared platform it's in our interest to modernise and make decisions about the frontend code there as much (if not more) than say MobileFrontend as this code is loaded in all our core projects. No team owns this code and no team will work on it if we don't. It also maps nicely to our existing goal T195473 by reducing our reliance on jQuery.

There are some more (Array or String) function from ES5 which can replace some jQuery functions like

  • $.grep -> Array.prototype.filter
  • $.inArray -> Array.prototype.indexOf
  • $.trim -> String.prototype.trim
Jdlrobson updated the task description. (Show Details)Aug 3 2018, 1:29 AM
ovasileva set the point value for this task to 8.Aug 7 2018, 5:00 PM
Nuria removed a subscriber: Nuria.Aug 7 2018, 5:10 PM
Krinkle moved this task from Inbox to Checkers on the MediaWiki-Core-Testing board.
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.

Change 451888 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Hygiene: Discourage use of $.each

https://gerrit.wikimedia.org/r/451888

Jdlrobson moved this task from Inbox to Focus on the User-Jdlrobson board.Aug 10 2018, 6:16 PM
Jdlrobson triaged this task as Normal priority.Aug 10 2018, 8:53 PM

Change 452860 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] WIP: Fix eslint warnings and switch to error code

https://gerrit.wikimedia.org/r/452860

Change 449670 merged by jenkins-bot:
[mediawiki/core@master] Warn against certain jQuery methods

https://gerrit.wikimedia.org/r/449670

Jdlrobson updated the task description. (Show Details)Aug 16 2018, 10:26 PM
Jdlrobson removed a project: Patch-For-Review.
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

Change 452860 merged by jenkins-bot:
[mediawiki/core@master] Fix eslint warnings and switch to error code

https://gerrit.wikimedia.org/r/452860

Change 456870 had a related patch set uploaded (by Fomafix; owner: Fomafix):
[mediawiki/core@master] Use Array.prototype.indexOf instead of $.inArray

https://gerrit.wikimedia.org/r/456870

Change 456870 abandoned by Fomafix:
Use Array.prototype.indexOf instead of $.inArray

Reason:
Duplicate to Idbd06e6a1681300c4ab9142c7b57e4376f474041. Add the eslint rule there.

https://gerrit.wikimedia.org/r/456870

Change 455255 had a related patch set uploaded (by Fomafix; owner: Petar.petkovic):
[mediawiki/core@master] Remove jQuery.inArray usages

https://gerrit.wikimedia.org/r/455255

Change 455255 merged by jenkins-bot:
[mediawiki/core@master] Remove jQuery.inArray usages

https://gerrit.wikimedia.org/r/455255

Change 457856 had a related patch set uploaded (by Fomafix; owner: Fomafix):
[mediawiki/core@master] Replace $.proxy by Function.prototype.bind

https://gerrit.wikimedia.org/r/457856

Jdlrobson updated the task description. (Show Details)Sep 5 2018, 5:43 PM

Change 457856 merged by jenkins-bot:
[mediawiki/core@master] Replace $.proxy by Function.prototype.bind

https://gerrit.wikimedia.org/r/457856

Change 451888 merged by jenkins-bot:
[mediawiki/core@master] Hygiene: Discourage use of $.each

https://gerrit.wikimedia.org/r/451888

Jdlrobson updated the task description. (Show Details)Sep 6 2018, 2:35 AM

Note also that $.map behaves differently from Array#map, in that returning null/undefined drops results. When replacing $.map make sure to check the return is never null/undefined, otherwise you will need an additional Array#filter call.

In T200877#4468482, @Jdlrobson added a project: Google-Code-in-2018.

@Jdlrobson: If we're left with updating the wikimedia eslint config to also warn for other extensions and skins this might welcome more info how to do this (presumably somehow in https://github.com/wikimedia/eslint-config-wikimedia/blob/master/wikimedia.json ?). Same for second left item.

Sure thing.

@Esanders is https://github.com/wikimedia/eslint-config-wikimedia/issues/106 the same as this task? I also want to make sure we dont have 2 bugs for the same thing.

Nirmos added a subscriber: Nirmos.Oct 11 2018, 11:23 PM

@Esanders: ping - can you please answer the last question?

Yes - this is the same as the GitHub issue.

Jdlrobson closed this task as Resolved.Nov 14 2018, 6:57 PM
Jdlrobson claimed this task.

Remaining work is being tracked on https://github.com/wikimedia/eslint-config-wikimedia/issues/106

Let me know if there is a phabricator task I can merge this into.

Jdlrobson updated the task description. (Show Details)Nov 14 2018, 6:57 PM