Page MenuHomePhabricator

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


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.


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.


  • Variables that can be local are not made global.

Notes on the use of globals


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


All the following can be trivially changed:

  • checkScroll
  • createPopupElement
  • createSVGMask


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


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


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.cache
  • mw.popups.render.renderers
  • mw.popups.render.render
  • mw.popups.render.reset


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

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

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

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

@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.