Page MenuHomePhabricator

Deprecate jquery.throttle-debounce in favour of mw.util.debounce or 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 (T236427 Dev: MobileFrontend should not use deprecated jquery.throttle-debounce)
    • RelatedArticles – ext.relatedArticles.readMore.bootstrap https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/RelatedArticles/+/545919
    • Translate – ext.translate.messagetable
    • Wikibase – jquery.wikibase.entityselector, wikibase.view.ViewFactory, and ext.WikidataPageBanner.positionBanner
    • WikiEditor – ext.wikiEditor
    • (skin) Vector – skins.vector.js https://gerrit.wikimedia.org/r/535914
    • (skin) MetroLook – skins.metrolook.js
  • Remove in ? MW 1.35.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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 TranscriptSep 5 2019, 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.Sep 6 2019, 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, Wiki-Techstorm-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.Sep 9 2019, 1:49 PM
Restricted Application added a project: Operations. · View Herald TranscriptSep 9 2019, 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, Wiki-Techstorm-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.Sep 9 2019, 3:55 PM
Jdforrester-WMF updated the task description. (Show Details)
DannyS712 removed GoogleLegacy as the assignee of this task.Sep 9 2019, 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.EditedSep 10 2019, 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.

I just attempted this, but noticed there is no util.throttle? This seems to block MobileFrontend removing this code.

$window
	.on( 'resize', apply2(
		util.debounce( 100, function () { eventBus.emit( 'resize' ); } ),
		$.throttle( 200, function () { eventBus.emit( 'resize:throttled' ); } )
	) )
	.on( 'scroll', apply2(
		util.debounce( 100, function () { eventBus.emit( 'scroll' ); } ),
		$.throttle( 200, function () { eventBus.emit( 'scroll:throttled' ); } )
	) );
Jdforrester-WMF renamed this task from Deprecate jquery.throttle-debounce in favour of OO.ui.debounce/throttle to Deprecate jquery.throttle-debounce in favour of mw.util.debounce or OO.ui.debounce/throttle.Oct 1 2019, 5:25 PM

@Jdlrobson I can elaborate N weeks from now, but T213426#5477587 might help get you on the way.

Xover added a subscriber: Xover.Wed, Oct 23, 7:58 AM

Change 545919 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/RelatedArticles@master] Use mw.util.debounce

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

Jdlrobson updated the task description. (Show Details)Thu, Oct 24, 6:49 PM
Jdlrobson removed a project: MobileFrontend.

Moved MobileFrontend to subtask.

Jdlrobson updated the task description. (Show Details)Thu, Oct 24, 6:52 PM

Change 545919 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles@master] Use mw.util.debounce

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

Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)