Page MenuHomePhabricator

Deprecate jquery.throttle-debounce in favour of OO.ui.debounce/throttle
Open, LowPublic

Description

Timeline:

  • Deprecate in MW 1.33
  • Remove uses around the codebase:
    • MediaWiki
      • mediawiki.htmlform.checker
      • mediawiki.page.gallery
      • mediawiki.special.userlogin.signup.js
    • CentralNotice – ext.centralNotice.adminUi.campaignManager
    • Flow – ext.flow.components and ext.flow
    • Graph – ext.graph.sandbox
    • InputBox – ext.inputBox
    • Kartographer – ext.kartographer.box and ext.kartographer.visualEditor
    • MassMessage – ext.MassMessage.special.js
    • MediaViewer – mmv
    • MobileFrontend – mobile.startup
    • RelatedArticles – ext.relatedArticles.readMore.bootstrap
    • Translate – ext.translate.messagetable
    • Wikibase – jquery.wikibase.entityselector, wikibase.view.ViewFactory, and ext.WikidataPageBanner.positionBanner
    • WikiEditor – ext.wikiEditor
    • (skin) Vector – skins.vector.js
    • (skin) MetroLook – skins.metrolook.js
  • Remove in ? MW 1.35.

Event Timeline

Esanders created this task.Jan 10 2019, 1:21 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 10 2019, 1:21 PM
$ mwgrep $.throttle
enwiki              MediaWiki:Gadget-ptoc.js
plwiktionary        MediaWiki:Gadget-persistent-toc.js
$ mwgrep $.debounce
commonswiki         MediaWiki:Gadget-catMoveLink.js
frwiki              MediaWiki:Gadget-CommonEdit.js
frwiki              MediaWiki:Gadget-ExternalSearch.js

Change 483407 had a related patch set uploaded (by Bartosz Dziewoński; owner: Esanders):
[mediawiki/core@master] Mark jquery.throttle-debounce as deprecated

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

I'm not sure if it is always desirable to replace jquery.throttle-debounce with OO.ui.debounce/throttle. jquery.throttle-debounce is a very small library compared to OOUI, so I would definitely use it if I didn't need anything else from OOUI. And I don't think it has any code quality problems that would warrant replacing it.

$ mwgrep $.throttle
enwiki              MediaWiki:Gadget-ptoc.js
plwiktionary        MediaWiki:Gadget-persistent-toc.js
$ mwgrep $.debounce
commonswiki         MediaWiki:Gadget-catMoveLink.js

All three of these gadgets are unused, AFAICT.

frwiki              MediaWiki:Gadget-CommonEdit.js
frwiki              MediaWiki:Gadget-ExternalSearch.js

These are "real" hits. The second one is mildly problematic from a privacy POV as it doesn't warn its users, but that's outside the scope of this task.

I'm not sure if it is always desirable to replace jquery.throttle-debounce with OO.ui.debounce/throttle. jquery.throttle-debounce is a very small library compared to OOUI, so I would definitely use it if I didn't need anything else from OOUI. And I don't think it has any code quality problems that would warrant replacing it.

That's a fair comment.

The general argument is that every RL module that exists in production slows down the site by a few dozen bytes for every reader for every page view.

The specific argument is about consolidation into a single set of code that we can change reliable everywhere for everyone when fixing bugs/adding new features. That said, I'm not aware of anything in this field that pushes for that. :-)

Change 483407 merged by jenkins-bot:
[mediawiki/core@master] Mark jquery.throttle-debounce as deprecated

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

Deprecate jquery.throttle-debounce in favour of OO.ui.debounce/throttle

Why? Where was the conversation that led to this decision? The description for this task is severely lacking for something that is now showing up in the developer console for all users. Please can I ask that we update it ASAP before the next train rolls out and this task gets more attention to explain the rationale for this change and the available migration paths.

I'm not sure if it is always desirable to replace jquery.throttle-debounce with OO.ui.debounce/throttle. jquery.throttle-debounce is a very small library compared to OOUI, so I would definitely use it if I didn't need anything else from OOUI. And I don't think it has any code quality problems that would warrant replacing it.

I totally agree. This doesn't seem very practical. throttle-debounce is used in MobileFrontend on the critical path inside mobile.startup
Adding "oojs-ui" as a dependency doubles the JS on the critical path from 58.9kb to 108kb (after gzip!). A 50kb library is not warranted for something so simple.
For MobileFrontend I imagine if this is indeed happening we'd swap it out for https://www.npmjs.com/package/throttle-debounce but I'm not sure it's useful for us to have 2 versions of debouncing code. Might it make sense to add this to mw e.g. mw.debounce ?

What other options are available as part of this deprecation process?

I think the logic warranted is sufficiently simple that the cheapest and most sustainable approach (in terms of maintenance cost, bandwidth, and run-time overhead), is to inline this utility in code bases that need it.

Any form of centralised abstraction means there's now an additional module to be registered (the cost to register it, and all dependency links to it, would actually start to compete with the size of the code itself). Not to mention that it's one more thing that can be broken, may need to be migrated/renamed, needs to be understood as an external package to familiarise yourself with.

These costs apply to all libraries of course. An when the library contributes significant value, benefits on-going maintenance in a problem space that is costly to keep up with, or solves a complicated problem that we may not get right the first time, then these end up with a net-positive value. For most smaller packages though, hardcoding them into place makes you free of bugs and worry, as the code is simple enough that it can't break after you test it at once, and no external factors could ever justify a meaningful change in it for you, because it works for what you need.

Fo something like a UI library that has ever-evolving needs for localisation, browser support, visual guidelines, and accessibility - the balance naturally ends up tipping the other way.


Example:

MobileFrontend/mobile.init.js
/* Wrap the function and debounce calls to it for a specified duration. */
function debounce( delay, callback ) {
	var timeout;
	return function debounced() {
		clearTimeout( timeout );
		timeout = setTimeout( Function.prototype.apply.bind( callback, this, arguments ), delay );
	};
}

$window.on( 'resize',  debounce( 100, function () { /* Respond to resize. */ } );

Sorry for not responding earlier; what Timo said. For almost everyone, who should already be using OOUI, the library provides its own version. The very few users for whom that is not appropriate should just in-line it.

Can this note from @Krinkle be added to the description and rationale for the deprecation be added to the task description? There's really no excuse for an empty task description, especially if we're pointing people to it directly from the console.

Schnark added a subscriber: Schnark.Mar 6 2019, 9:15 AM

Can this note from @Krinkle be added to the description and rationale for the deprecation be added to the task description? There's really no excuse for an empty task description, especially if we're pointing people to it directly from the console.

Especially since currently the rationale seems to be that there only 5 gadgets using this, while in reality there are 16 (or such) extensions, most of them deployed on WMF wikis, which use that module and now start to spit out deprecation warnings on the console.

Change 495028 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] Revert "Mark jquery.throttle-debounce as deprecated"

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

@Krinkle The library is short, but I don't think it's simple enough that it should be inlined everywhere. It's a tricky implementation and I would prefer to keep it centralized. A similar example is the mw.RegExp.escape function, for which we have a separate module.

@Krinkle The library is short, but I don't think it's simple enough that it should be inlined everywhere. It's a tricky implementation and I would prefer to keep it centralized. A similar example is the mw.RegExp.escape function, for which we have a separate module.

The escaping mechanism is security sensitive, potentially variable over time, and not domain-specific knowledge for a specific feature. I would also agree it's tricky to get right (and you won't know when it's wrong per se). On the other hand, doing something like "call setTimeout and ignoring successive calls", is domain logic for specific your feature. When you inline it, you decide what you need it to do, and will never have to change it after that. I don't agree it's tricky. And you'd know if it's not working right.

The few files that use logic like this are unlikely to be on the same page. And even if they are, we're talking about ~ 60 bytes (data), which is less than ~ 550 bytes. We'd need to have 9 different features use it, before we're even. And then we're still not really even, given those would load deferred and in parallel, instead of blocking. If we do find the cost a problem, there's plenty of things we can do to free up that budget by making other changes (I'm happy to help).

If we use a central module, the cost distributes differently. The registration in the startup module needs its name, version, and all dependency links from other modules. Currently:

.., ["jquery.throttle-debounce","0z1cny2"], .. # registration (39 bytes)
.., ["mediawiki.htmlform.checker","0ja58f6",[46]], .. #
.., ["skins.vector.js","0xzs1dv",[42,46]], .. # dependencies (enwiki: 13x, 3-5 bytes, let's say 3. 13 x 3 = 39)

That's 78 bytes. Pretty close to 60 bytes, but remember that we haven'y downloaded the code yet. It's also part of a larger principal: the registry isn't about jquery debounce, it's about all these kinds of little functions. These start to add up. Note that the startup module has to complete before we can even start loading actual code. The jquery.throttle-debounce module would add another 480 bytes. (data)

In conclusion, if weuse jquery.throttle-bounce

  • Depends from 13 modules. Load one of them.
  • Pay 480+78 = 558 bytes. (partly blocking interaction)
  • Can't change it. Risk of feature creep and generalisation.

Compare to inline:

  • No deps, in one loaded module.
  • Pay 64 bytes. (non-blocking)
  • Locally free to change. Low risk.
Krinkle added a comment.EditedMar 7 2019, 9:19 PM

About mw.RegExp.escape, per the above, it is imho worth keeping centralised. But, I do think it was a mistake (my mistake), to have as its own module bundle. It'd make a lot more sense to bundle with something else, e.g. mediawiki.util. It's not worth the cost of registering as its own entry point / module bundle.

I still think it's a mistake in this case to prioritize performance over maintainability but it's a reasonable decision to make. Clearly I'm not going to convince you, and I don't really care that much.

Change 495028 abandoned by Bartosz Dziewoński:
Revert "Mark jquery.throttle-debounce as deprecated"

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

Krinkle triaged this task as Low priority.Mar 28 2019, 7:54 PM
Krinkle updated the task description. (Show Details)

I'm still having issue in visualization, I get the console message of deprecation of jquery.throttle-debounce and to use OO.ui.throttle/debounce instead. I don't know what I have to do to fix this issue, my application looks broken. I have the page contents at top, and the sidebar at bottom. Always looks working with a such of mobile layout. I'm following the current git version, and I have tried to pull a new clear repository, same version. I have not tried the downgrade, the issue is not so critical to try a downgrade. Any hints to fix this?

I'm still having issue in visualization, I get the console message of deprecation of jquery.throttle-debounce and to use OO.ui.throttle/debounce instead. I don't know what I have to do to fix this issue, my application looks broken. I have the page contents at top, and the sidebar at bottom. Always looks working with a such of mobile layout. I'm following the current git version, and I have tried to pull a new clear repository, same version. I have not tried the downgrade, the issue is not so critical to try a downgrade. Any hints to fix this?

Hey there, the deprecation only means that the message will show up in the console (and that we'll remove it in a future version of MediaWiki). Anything that has changed about how your code works will be unrelated to this, sorry.

Since the last git pull I get an apparently broken web interface, I'm using the theme Vector. I tried to switch to another, the Material theme that looks very nice, but I get the same defect. I don't know how to debug it and all I can see is this new warning in console.
Looking into ssl_error_log in my server (an updated Centos7) I don't see any message, the only record I see is by enabling the plugin TinyMCE, that don't make any difference on the web application, is:
[Fri May 10 12:35:51.391763 2019] [php7:notice] [pid 23215] [client 10.199.180.80:56048] PHP Notice: Undefined property: Parser::$mUniqPrefix in /var/www/html/wiki/extensions/TinyMCE_MW/TinyMCE_MW.php on line 285, referer: https://10.199.180.172/wiki/index.php/Pagina_principale
Of course I tried the composer update and php maintenance/update.php, I usually do it when do git pull into the wiki directory, I also tried to download a new copy of the git wiki, and then run a new composer update that download again the dependency but I get the same result.
It's disarming, I just don't know how fix.
I suspect the problem is somewhere in the database, but don't know a maintenance job to scan.

@Rivaldid: That sound not directly related and like a question for the support desk?

Well, I have just compared a dump of the db before the update which I'm speaking, and a current one and there are no significant changes, I just see new insert into and a table in which a type blob is being mediumblob. The only changes I've done are a server update and a git update. In the server update I see httpd, httpd-tools, composer. The composer update has made me think to the wiki update with his composer dependency. There are other packages but I don't thought they are related to the web server.
Sorry if this is not the right place where ask but I have begun here the description of my issue. What can I do to ask the fix? I'm not so practiced with bug trackers and support services. Sorry again.

ah, wow. I fixed my wiki. I never thought to update the theme, in this case Vector. Also my theme comes from his git repository. All I've done is a git pull. So my issue was not related to this deprecation as you told before, it was just a version mismatching with the theme. Sorry and thanks.

Change 534686 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] mediawiki.htmlform.checker: Remove use of deprecated jquery.throttle-debounce

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

Restricted Application added a project: Growth-Team. · View Herald TranscriptThu, Sep 5, 10:06 PM

This is definitely not going to happen in the next few weeks. :-)

Change 534686 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.htmlform.checker: Remove use of deprecated jquery.throttle-debounce

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

kostajh moved this task from Inbox to FY 2019-20 on the Growth-Team board.Fri, Sep 6, 9:51 AM
GoogleLegacy raised the priority of this task from Low to High.
GoogleLegacy updated the task description. (Show Details)
GoogleLegacy removed subscribers: Galobtter, Rivaldid, Schnark and 7 others.
GoogleLegacy added a subscriber: GoogleLegacy.
GoogleLegacy added projects: GerritBot, Jenkins, ci-test-error, meta-jenkins-test, Accuracy-Review-of-Wikipedias, wikipedia.de, Wikipedia-Requests-App, Wikipedia-Android-App-Backlog, wikitech.wikimedia.org, TechCom-RFC, Wikistorm-2018, Wikistorm-2019, Wikibugs, Tool-Labs-tools-wikibugs-IRC-bot, Wikimedia-wikibugs-IRC-bot, WikiApiary, WikiArticleFeeds, WMSE-Bug-Reporting-and-Translation-2019, wb_terms - Tool Builders Migration, Better Use Of Data, Bugzilla-Migration, Bad-Words-Detection-System, SpamBlacklist, SpamRegex, Trash, PasswordlessLogin, MediaWiki-extensions-PasswordReset, media-reports-tool, WMSE-FindingGLAMs-2018 (Media uploads), MediaWiki-Cache, MediaWiki-Comment-backend, MediaWiki-Containers, Measuring-value-added, Google-Code-in-2019, Google-Drive-to-Commons, Google Season of Docs 2019, Google-Summer-of-Code, GoogleLogin, Wikipedia-iOS-App-Backlog (Administrative), acl*blog-admins, acl*discovery-repository-admins, acl*otrs-admins, acl*Project-Admins, GLAM, GLAM-Tech, glam2commons, WMSE-GLAM-2019, FR-Civi-Dedupe, Fr-fortnightly, FR-Q2-FY2019-20-cleanup-list, acl*WMF-FR, Bot-Frameworks, Fundraising Sprint King Kong vs. Mozilla, acl*stewards, Code-Stewardship-Reviews, Stewards-and-global-tools (Temporary-UserRights), Tool-stewardbots, GitHub-Mirrors, Github-notif-bot, Gitblit, Gitblit-Deprecate, Connected-Open-Heritage-Wikidata-migration (am-hy), Mobile-App-Android-Sprint-95-Americium, Brickimedia, Browser-Support-Android-Google-Chrome, Browser-Support-Apple-Safari, Browser-Support-Firefox, Browser-Support-Google-Chrome, Wiki-goes-Caribbean, Wiki-Loves-Love, WMSE-Wiki-Loves-2019 (Wiki Loves Monuments 2019), Wiki-Project-Med, Reading Epics (Trending Edits), Trending-Service, Trebuchet, MinervaNeue (Tracking), Page-Previews (Tracking), Readers-Web-Backlog (Tracking), Tracking-Neverending, Traffic.Mon, Sep 9, 1:49 PM
Restricted Application added a project: Operations. · View Herald TranscriptMon, Sep 9, 1:49 PM
GoogleLegacy rescinded a token.
Jdforrester-WMF removed projects: Operations, Traffic, Tracking-Neverending, Readers-Web-Backlog (Tracking), Page-Previews (Tracking), MinervaNeue (Tracking), Trebuchet, Trending-Service, Reading Epics (Trending Edits), Wiki-Project-Med, WMSE-Wiki-Loves-2019 (Wiki Loves Monuments 2019), Wiki-Loves-Love, Wiki-goes-Caribbean, Browser-Support-Google-Chrome, Browser-Support-Firefox, Browser-Support-Apple-Safari, Browser-Support-Android-Google-Chrome, Brickimedia, Mobile-App-Android-Sprint-95-Americium, Connected-Open-Heritage-Wikidata-migration (am-hy), Gitblit-Deprecate, Gitblit, Github-notif-bot, GitHub-Mirrors, Tool-stewardbots, Stewards-and-global-tools (Temporary-UserRights), Code-Stewardship-Reviews, acl*stewards, Fundraising Sprint King Kong vs. Mozilla, Bot-Frameworks, acl*WMF-FR, FR-Q2-FY2019-20-cleanup-list, Fr-fortnightly, FR-Civi-Dedupe, WMSE-GLAM-2019, glam2commons, GLAM-Tech, GLAM, acl*Project-Admins, acl*otrs-admins, acl*discovery-repository-admins, acl*blog-admins, Wikipedia-iOS-App-Backlog (Administrative), GoogleLogin, Google-Summer-of-Code, Google Season of Docs 2019, Google-Drive-to-Commons, Google-Code-in-2019, Measuring-value-added, MediaWiki-Containers, MediaWiki-Comment-backend, MediaWiki-Cache, WMSE-FindingGLAMs-2018 (Media uploads), media-reports-tool, MediaWiki-extensions-PasswordReset, PasswordlessLogin, Trash, SpamRegex, SpamBlacklist, Bad-Words-Detection-System, Bugzilla-Migration, Better Use Of Data, wb_terms - Tool Builders Migration, WMSE-Bug-Reporting-and-Translation-2019, WikiArticleFeeds, WikiApiary, Wikimedia-wikibugs-IRC-bot, Tool-Labs-tools-wikibugs-IRC-bot, Wikibugs, Wikistorm-2019, Wikistorm-2018, TechCom-RFC, wikitech.wikimedia.org, Wikipedia-Android-App-Backlog, Wikipedia-Requests-App, wikipedia.de, Accuracy-Review-of-Wikipedias, meta-jenkins-test, ci-test-error, Jenkins, GerritBot.Mon, Sep 9, 3:55 PM
Jdforrester-WMF updated the task description. (Show Details)
DannyS712 removed GoogleLegacy as the assignee of this task.Mon, Sep 9, 4:36 PM
DannyS712 lowered the priority of this task from High to Low.
DannyS712 edited subscribers, added: Galobtter, Rivaldid, Schnark and 7 others; removed: GoogleLegacy.
DannyS712 added a subscriber: GoogleLegacy.
DannyS712 removed a subscriber: GoogleLegacy.

I think a mw.util method would be the best move here given the 3 usages in core and given the minimal implementation of https://gerrit.wikimedia.org/r/534686 - I don't think we'll want to copy-paste that function into 3 places in core will we?

Krinkle added a comment.EditedTue, Sep 10, 2:36 AM

I've often found it a sign of technical debt and think it's worth taking this one slow (could even undo the deprecation warning for now). From a quick scan it looks like nothing should need it on regular page views before interaction, in which case mw.util would needlessly add more to the payload.

Having said that, if it's only debounce (not throttle) and we do find unavoidable uses in core or bundled extensions that need it on every page view, then adding to mw.util would indeed be the cheapest and most straight-forward way to ship it to the client.

EDIT: Never mind. I was thinking of throttle as the one that seems better implemented natively in a piece of code (where you explicitly have a variable saying "ignore this" thats raised at the start and lowered when the async step or timeout is done). But for pure debounce indeed seems like the code only gets more obscure if you try to do it ad-hoc, except as a separate utility, which we might as well centralise at that point.

Let's do it :)

Change 535759 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] mediawiki.util: Add debounce() function

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

Change 535761 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] mediawiki.special.userlogin.signup: Remove unused debounce dependency

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

Change 535761 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.special.userlogin.signup: Remove unused debounce dependency

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

Change 535914 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/skins/Vector@master] Use mw.util.debounce() in collapsibleTabs.js

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

Change 535759 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.util: Add debounce() function

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

Change 535914 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Use mw.util.debounce() in collapsibleTabs.js

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

This comment was removed by Aklapper.
This comment was removed by Aklapper.