Page MenuHomePhabricator

JSDoc of mw.notification.notify is incorrect
Open, Needs TriagePublic

Description

https://doc.wikimedia.org/mediawiki-core/master/js/mw.notification.html#.notify

returns "Notification". This gets linked to the webapi https://developer.mozilla.org/en-US/docs/Web/API/Notification

It is however the internal Notification object: https://doc.wikimedia.org/mediawiki-core/master/js/mediawiki.notification_notification.js.html#line25

which is hidden from jsdoc because "The constructor is not publicly accessible; use [mw.notification.notify]{@link mw.notification} instead."

It's strange that we hide an internal object, that is part of the public API, and link to incorrect unrelated documentation at the same time.

Event Timeline

apaskulin subscribed.

which is hidden from jsdoc because "The constructor is not publicly accessible; use [mw.notification.notify]{@link mw.notification} instead."

It looks like the reason that this is being hidden from JSDoc is because the theme's externals plugin is overriding the creation of the Notification page with the Mozilla link. If I disable the plugin locally, JSDoc generates a Notification page and links to it from mw.notification. See https://github.com/wikimedia/jsdoc-wmf-theme/blob/master/src/defaultMaps.js#L100-L104 and https://github.com/wikimedia/jsdoc-wmf-theme/blob/master/plugins/externals.js.

The quickest fix seems to be to add "Notification": "Notification.html" to the linkMap in the core JSDoc config. This fixes the issue for me locally. Updated: This approach doesn't actually work.

The quickest fix seems to be to change Notification to mw.Notification and remove the @global tag from its doc block. This allows an mw.Notification page to be created and linked to. I assume this would be ok since this was the name of the class in JSDuck. Note that in order to see the type details, we would also need to remove the @hideconstructor tag or create a separate doc block for the type.

A more robust fix would be to modify the externals plugin to treat false values in the linkMap the same way it does for the prefixMap so we could instead fix the issue by adding "Notification": false in the core JSDoc config. I haven't tested this locally yet.

I'm in favor of implementing the quick fix now and opening a task for the more robust fix. Thanks for catching this, @TheDJ!

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

[mediawiki/core@master] docs: Fix link to mw.Notification in JSDoc

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

Test wiki created on Patch demo by APaskulin (WMF) using patch(es) linked to this task:
http://patchdemo.wmcloud.org/wikis/7aab404007/w/

Change #1127535 abandoned by Alex Paskulin:

[mediawiki/core@master] docs: Fix link to mw.Notification in JSDoc

Reason:

Thanks for the review, TheDJ. Abandoning since this approach doesn't work

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

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

[mediawiki/core@master] docs: Make internal Notification object visible on docs site

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

Test wiki created on Patch demo by APaskulin (WMF) using patch(es) linked to this task:
https://patchdemo.wmcloud.org/wikis/5abcdf2b99/w/

The quickest fix seems to be to change Notification to mw.Notification and remove the @global tag from its doc block.

I experimented with this approach, but it caused the namespace page to move from .../js/mw.notification.html to .../js/mw.notification_.html with an extra underscore. Everything within the site seemed to link to the correct pages still, but I thought this was likely create confusion in some way.

Instead, I submitted an experimental patch that creates a typedef on the namespace page. This fulfills the original issue of the task without creating confusion on the site between a mw.Notification class page and an mw.notification namespace page. Although I'm not sure which approach is better. Thoughts welcome!