Page MenuHomePhabricator

Clarify stability of mw.loader.addStyleTag() in documentation
Closed, ResolvedPublic

Description

Can you re-surface mw.loader.addStyleTag()? It's been quite handy, it's used a lot, and the doc for mw.util.addCSS() even mentions it as an alternative.

Event Timeline

addStyleTag() is defined in mediawiki.loader.js#L1351 as an alias for newStyleTag(). But since newStyleTag() is marked as private, we can't just add an @alias tag. To add addStyleTag() to the docs, we'll either need to duplicate the doc block or make newStyleTag() public.

Jdlrobson subscribed.

We talked about this in our triage meeting (@AnneT @Catrope .@apaskulin). We recommend using the mw.util.addCSS method, mw.loader.addStyleTag although used should not be considered a stable function. We don't plan to deprecate it but we shouldn't advertise it in the documentation. We'll update the documentation to remove this line.

Jdlrobson renamed this task from Re-surface mw.loader.addStyleTag() to Remove link to mw.loader.addStyleTag() in documentation.Feb 29 2024, 10:13 PM
Jdlrobson claimed this task.
Jdlrobson triaged this task as Medium priority.
Jdlrobson moved this task from Backlog to Migration of core docs on the JSDoc WMF theme board.

Wow, you can do that? If you could mark as private what was marked as stable (and thus make it possible to make breaking changes to it without going through the deprecation process), wouldn't that provide a loophole to the stable interface policy?

Wow, you can do that? If you could mark as private what was marked as stable (and thus make it possible to make breaking changes to it without going through the deprecation process), wouldn't that provide a loophole to the stable interface policy?

From https://www.mediawiki.org/wiki/Stable_interface_policy/Frontend "The mw object is the primary stable API for interacting with MediaWiki in the browser." and "Review doc.wikimedia.org (particularly the MediaWiki core documentation) for documentation of the available stable interfaces."

However, part of the motivation for the migration from JSDuck to JSDoc was to do a final review of what code should be considered stable and what should not.
This is explicitly flagged in this warning box in https://www.mediawiki.org/wiki/Stable_interface_policy/Frontend#JavaScript_code

Screenshot 2024-02-29 at 4.16.40 PM.png (490×1 px, 175 KB)

In this case, "We aim to clarify stable contracts where ambiguous before the 1.42 release " applies. The stability of this method is ambiguous as in the code it hasn't been marked as @stable or @private (it hasn't even been documented).

newStyleTag has been tagged as @private https://github.com/wikimedia/mediawiki/blob/master/resources/src/startup/mediawiki.loader.js#L188
The privacy of addStyleTag has not been declared https://github.com/wikimedia/mediawiki/blob/master/resources/src/startup/mediawiki.loader.js#L1349

Given an alternative exists we are recommending that we tag this as @private as part of review. I'd rather we don't have multiple ways of doing things in our codebase.

(Clarifying Note: that doesn't mean we shouldn't go through the deprecation process for this method if we ever decide to remove it)

The stability of this method is ambiguous as in the code it hasn't been marked as @stable or @private (it hasn't even been documented).

It was documented until you removed it in rMW9f2e4faea3ff. Before that, it was explicitly non-private, as evident by comparing it to the other "add..." methods under mw.loader.

But more importantly, whether it was intended or not, the effect of the previous doc is that it has been seen as a stable interface, so breaking changes to it would be just as disruptive as to any other (so-documented) stable interface, and that should be noted in the code at the very least.

Change 1007939 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Mark addStyleTag as for internal usage

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

Change 1007940 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Mark addStyleTag as public

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

The stability of this method is ambiguous as in the code it hasn't been marked as @stable or @private (it hasn't even been documented).

It was documented until you removed it in rMW9f2e4faea3ff. Before that, it was explicitly non-private, as evident by comparing it to the other "add..." methods under mw.loader.

While I can't argue this appeared in the old documentation, I don't agree that this is not ambiguous. Firstly the method addScriptTag next to it is exposed for internal use only. The method addStyleTag has a line @inheritdoc #newStyleTag - if it's inheriting documentation I would also expect it to inherit @private (it seems as you point out JSDuck treats inheritance of @private different to jsdoc.

TBH I don't feel strongly abut the outcome, only that this should be made more explicit in the documentation so either https://gerrit.wikimedia.org/r/1007939 or https://gerrit.wikimedia.org/r/1007940

Looking more closely at the code for addCSS, that function calls addStyleTag with the only difference being the return value.
@Catrope @AnneT what do you think?

Jdlrobson renamed this task from Remove link to mw.loader.addStyleTag() in documentation to Clarify stability of mw.loader.addStyleTag() in documentation.Mar 1 2024, 4:14 PM

(Re-tagging our Inbox as the conversation shifted from a routine doc fix, to removing a public API.)

[…] I don't agree that this is not ambiguous. Firstly the method addScriptTag next to it is exposed for internal use only. […]

When writing code, there is generally not a way to order code such that there isn't somewhere a public function next to a private one. We don't typically group code holistically by visibility in MediaWiki. You'll see a bunch of related private ones together and some related public ones together, but rarely as the top-level organisation. But, if one were to choose that as your way of organising, I believe there would still be one, next to, the other.

The proximity to a private method can of course still be interesting, and could be a reason to take a closer look. There is after all the chance someone forgot to mark it as internal. However, when we have evidence on one side, and an unproven possibility of a mistake therein, that is not ambigious.

So, let's use the proximity as prompt to take a closer look. What information is available to us about mw.loader.addStyleTag? I'm starting at https://github.com/wikimedia/mediawiki/blob/1.41.0/resources/src/startup/mediawiki.loader.js#L1346-L1355

	/**
	 * @inheritdoc #newStyleTag
	 * @method
	 */
	addStyleTag: newStyleTag,

	// Exposed for internal use only.
	addScriptTag: addScript,
	addLinkTag: addLink,
  • mw.loader.addStyleTag is documented with an explicit /** @method */ doc block. There is no requirement for an object literal to have a doc block on a key, and indeed we have no internal APIs (in this file, or otherwise in core) that publicly expose an already-documented functions with another doc block. This one has a doc block. One could "blame" the code to learn why. Commit message guidelines help with this.
  • mw.loader.addStyleTag has no internal references. Internal code calls the private function directly. This begs the question why it is exposed. Seemingly not for internal use.
  • mw.loader.addStyleTag is explicitly referred to and recommended in the description of another public API, mw.util.addCSS. That's a pretty strong indicator. If in doubt, one could find how this recommendation was added.
  • mw.loader.addStyleTag is defined separately, and under a different name ("addStyleTag" vs "newStyleTag"), thus distinguishing itself in some way, for some reason. There is no requirement to have two names. This one does. That's another thing to make one wonder if there's more to it.
  • If one were to search via Git blame, one would find change 67648 which adds the doc block and explains why.

Change 1007939 abandoned by Catrope:

[mediawiki/core@master] Mark addStyleTag as for internal usage

Reason:

Per comments we'll do https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1007940 instead

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

Thanks for the context, all. I think both points here are valid. Looking at the doc blocks, it's unclear whether addStyleTag inherits @private or just the description and parameters from newStyleTag. I also agree that it would be ideal to de-duplicate the functionality between mw.loader.addStyleTag and mw.util.addCSS. However, it seems that addStyleTag was presented as public in the JSDuck documentation, so we should present the same thing in JSDoc.

If it's ok to remove @private from newStyleTag, I'm in favor of doing that and adding @name mw.loader.addStyleTag as done in 1007940. Otherwise, if newStyleTag needs to stay private, the only way to have addStyleTag appear in JSDoc is to duplicate the doc block.

Change 1007940 merged by jenkins-bot:

[mediawiki/core@master] Mark addStyleTag as public

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

Change 1009626 had a related patch set uploaded (by Alex Paskulin; author: Alex Paskulin):

[mediawiki/core@master] docs: Document addStyleTag as a method

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

Change 1009626 merged by jenkins-bot:

[mediawiki/core@master] docs: Document addStyleTag as a method

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

Thank you! The link in addCSS() is still broken, but I assume cross-references just haven't been got around to.