Page MenuHomePhabricator

Limit the number of globals in the Popups JS codebase
Closed, DuplicatePublic

Description

There are many cases where local functions or variables are being exposed as part of the mw.popups object. We should go over the codebase and make those functions and variables local.

From https://gerrit.wikimedia.org/r/#/c/313258/4/resources/ext.popups.renderer/desktopRenderer.js:

In a follow up can we take the opportunity to stop clickHandler and leaveInactive from being a global?
It's really confusing as it's only used in this file so no need to be exposed anywhere.

A/C

  • Variables that can be local are not made global.

Notes on the use of globals

ext.popups.core

The following globals encourage the renderer to contain logic and to act like a controller (getting the information) and with some refactoring do not need to be global. Many are also exposed for testing purposes. We should reconsider what, why and how we test these things

  • mw.popups.scrolled
  • mw.popups.IGNORE_CLASSES
  • mw.popups.removeTooltips - exposed for testing but shouldn't be a global
  • mw.popups.setupTriggers - feels like this is part of an init method.
  • mw.popups.getTitle
  • mw.popups.api
  • mw.popups.getAction
  • mw.popups.getRandomToken - currently exposed by ext.popups.core but only used by ext.popups.schemaPopups.utils
  • mw.popups.getPreviewCountBucket - currently exposed by ext.popups.core but only used by ext.popups.schemaPopups.utils
  • mw.popups.incrementPreviewCount - currently exposed by ext.popups.core but only used by ext.popups.renderer.desktopRenderer

This should probably be moved to ext.popups.settings.js or their own dedicated module. isUserInCondition is only exposed for testing purposes.

  • mw.popups.saveEnabledState
  • mw.popups.getEnabledState
  • mw.popups.isUserInCondition

ext.popups.targets.desktopTarget

All the following can be trivially changed:

  • checkScroll
  • createPopupElement
  • createSVGMask

ext.popups.schemaPopups.utils

This appears to be exposed for the purpose of testing and for use inside ext.popups.schemaPopups We could use module.exports instead to avoid population of the global mw.popup and explore merging the two ResourceLoader modules.

  • mw.popups.schemaPopups

ext.popups.desktop

Probably shouldn't expose any globals yet it exposes the following:

  • mw.popups.render.abortCurrentRequest
  • mw.popups.render.getClosestYPosition

It overrides mw.popups.render.renderers.article. it would be better to do something explicit such as mw.popups.registerRenderer

  • mw.popups.render.renderers.article

ext.popups.renderer.desktopRenderer

openPopup and closePopup should arguably be the only globally exposed methods.

  • mw.popups.render.POPUP_DELAY
  • mw.popups.render.POPUP_CLOSE_DELAY
  • mw.popups.render.API_DELAY
  • mw.popups.render.DWELL_EVENTS_MIN_INTERACTION_TIME
  • mw.popups.render.cache
  • mw.popups.render.renderers
  • mw.popups.render.render
  • mw.popups.render.reset

Details

Related Gerrit Patches:
mediawiki/extensions/Popups : masterDo not unnecessarily expose globals
mediawiki/extensions/Popups : masterEvent handlers should not be exposed globally

Event Timeline

bmansurov created this task.Oct 4 2016, 1:14 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 4 2016, 1:14 PM
Jhernandez added a subscriber: Jhernandez.
jhobs triaged this task as Medium priority.Oct 5 2016, 5:12 PM
jhobs moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.

Change 315582 had a related patch set uploaded (by Jdlrobson):
Event handlers should not be exposed globally

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

Change 315585 had a related patch set uploaded (by Jdlrobson):
Wait and reset do not need to be exposed globally

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

Change 315582 merged by jenkins-bot:
Event handlers should not be exposed globally

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

bmansurov updated the task description. (Show Details)Oct 14 2016, 4:28 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

Change 315585 abandoned by Jdlrobson:
Do not unnecessarily expose globals

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

@bmansurov - what remains to be done on this one?

@ovasileva this needs analysis since we're rewriting most of the codebase. I'm even tempted to close this as invalid once we merge the new code into master.

ovasileva changed the task status from Open to Stalled.EditedDec 19 2016, 8:06 PM

@bmansurov - agreed - let's mark as stalled for now

@phuedx - is this still valid? Can we close?

Let's look at this after the rewrite has been merged.