Page MenuHomePhabricator

Deprecate 'View.$'
Closed, ResolvedPublic3 Estimated Story Points

Description

Having a property with the name $ is confusing because it does not well describe what the method does.

The name was probably chosen because it behaves like one of the many overloaded features of window.$/jquery, specifically you can pass in a selector and get a collection, e.g. this.$( '.class' ). However it is not clear that this.$( '<span>' ) and this.$( fn ) will not work.

An added complication is that OOUI used to define a property $ which was the full jQuery global, but bound to the local iFrame's window. We deprecated this method a while ago when we dropped our iFrame approach for dialogs, but the property still exists as an alias to window.$

As the definition of the function is just a mapping to this.$el.find, so I would suggest renaming to this.find or this.findInEl, or simply not replace it at all: this.$el.find is not that long, and makes it completely clear what is happening.

We also now have a linting rule that prevents users from using global selectors at all ( $( '.selector' ) ), which I imagine another reason why this method was implemented.

edit: Bonus reason to do this is 'this.$' messes with eslint-plugin-jquery, which doesn't recognise it as returning a jQuery collection.

Event Timeline

Shouldn't be a problem in the wild:

> mwgrep "(this|self)\.\$[^a-zA-Z]"
jawiki              MediaWiki:Gadget-jastyle.js
testwiki            MediaWiki:Gadget-teahouse/ui/dialog/Question.js
testwiki            MediaWiki:Gadget-teahouse/ui/dialogs/th.ui.QuestionDialog.js

(total: 3, shown: 3)

edit: More generically:

mwgrep "\.\$\("
## Public wiki results
commonswiki         MediaWiki:Gadget-ImageAnnotator.js
commonswiki         MediaWiki:LAPI.js
dewikiversity       MediaWiki:LAPI.js
enwiki              MediaWiki:Gadget-ImageAnnotator.js
eowiki              MediaWiki:Gadget-ImageAnnotator.js
frwiki              MediaWiki:Gadget-WikEd/local.js
gomwiki             MediaWiki:Gadget-ImageAnnotator.js
huwiki              MediaWiki:Gadget-ImageAnnotator.js
jawiki              MediaWiki:Gadget-jastyle.js
knwiki              MediaWiki:Gadget-ImageAnnotator.js
mswiki              MediaWiki:Gadget-ImageAnnotator.js
pswiki              MediaWiki:Gadget-ImageAnnotator.js
rowiki              MediaWiki:Gadget-ImageAnnotator.js
ruwikibooks         MediaWiki:Gadget-ImageAnnotator.js
urwiki              MediaWiki:Gadget-ImageAnnotator.js

## Private wiki results
officewiki          MediaWiki:Gadget-ImageAnnotator.js
(total: 16, shown: 16)

These mostly refer to LAPI.$, and then some mystery broken code on jawiki, so looks like nothing else to fix.

Provided this is done via the deprecation process I have no problems with this. Is this something your team could do Ed?

Jdlrobson renamed this task from Rename or remove 'this.$' to Rename 'this.$' to more descriptive 'this.findInElement.Feb 15 2019, 6:28 PM
Jdlrobson triaged this task as Medium priority.
Jdlrobson moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.

Yeah - shouldn't be too hard. Just wanted to check if you had any objections.

My preference would be to remove the method, i.e. replace with this.$el.find...

Change 490904 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/skins/MinervaNeue@master] Avoid deprecated View.$

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

Change 490909 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/MobileFrontend@master] [DEPRECATING CHANGE] Deprecate View.$

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

Change 490910 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/ExternalGuidance@master] Avoid deprecated View.$

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

Esanders renamed this task from Rename 'this.$' to more descriptive 'this.findInElement to Rename or remove 'this.$'.Feb 15 2019, 7:25 PM
Esanders claimed this task.
Esanders moved this task from Incoming to Code review on the VisualEditor (Current work) board.

Change 490910 merged by jenkins-bot:
[mediawiki/extensions/ExternalGuidance@master] Avoid deprecated View.$

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

Please allow others to chip in per T216264#4957834 before further merges.

Agreed, it was mistakenly confused for an OOUI-related issue.

I believe the this this task refers to is View and I'm 100% on board. This issue has caused me lots of confusion and I've brought it up in our team Slack channel previously. I would love to see View.$() disappear or, if necessary, be renamed to View.find(). I don't think it should be a needed API if we are going to expose View.$el and would prefer, if any usage be retained, for it to be migrated to util.js.

I believe the this this task refers to is View and I'm 100% on board. This issue has caused me lots of confusion and I've brought it up in our team Slack channel previously. I would love to see View.$() disappear or, if necessary, be renamed to View.find(). I don't think it should be a needed API if we are going to expose View.$el and would prefer, if any usage be retained, for it to be migrated to util.js.

Agree with what @Niedzielski said. Am for this change

Esanders renamed this task from Rename or remove 'this.$' to Rename or remove 'View.$'.Feb 19 2019, 10:51 PM

I believe the this this task refers to is View and I'm 100% on board.

Good point, I used View.$ in the patches and have updated this task.

Jdlrobson set the point value for this task to 3.Feb 20 2019, 5:27 PM

We chatted about this today and it looks like we have no qualms about this change and reviewing it.

Change 490904 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Avoid deprecated View.$

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

Change 490909 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] [DEPRECATING CHANGE] Deprecate View.$

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

(We are expecting QA to be done by editing team here but please let me know if that is not the case!)

Change 495314 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/ExternalGuidance@master] mw.externalguidance.special/createpage: Avoid deprecated View.$

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

The task says we want to remove View.$, but so far we have only deprecated it. Do we want to have a longer deprecation period (and rephrase this task to be about deprecating), or should we go ahead and remove it now (and keep this task open until that is done)?

I searched across all extensions for .$( and .$. to try to find potential uses of this. Except for many false positives from other libraries, I found one (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ExternalGuidance/+/495314), and also one that uses OOUI's Element.$ (which is unrelated, but also deprecated; https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Kartographer/+/495313).

Change 495314 merged by jenkins-bot:
[mediawiki/extensions/ExternalGuidance@master] mw.externalguidance.special/createpage: Avoid deprecated View.$

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

Esanders renamed this task from Rename or remove 'View.$' to Deprecate 'View.$'.Mar 11 2019, 4:29 PM