Page MenuHomePhabricator

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

Description

Timeline:

  • Deprecate in MW 1.33
  • Remove in MW 1.34.

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 updated the task description. (Show Details)Thu, Mar 28, 7:54 PM
Krinkle triaged this task as Low priority.