Page MenuHomePhabricator

Popups render method should be dumb
Closed, ResolvedPublic

Description

Currently the renderer for Popups mixes view and controller logic.

  1. Code in ext.popups.core.js calls a render method
  2. The mw.popups.render.render method makes an api request via a call to mw.popups.render.renderers.article.init
  3. When this is completed it makes a call to mw.popups.render.openPopup
  4. The mw.track method is called in [[ URL | various ]] places by the renderer - a render cannot be called without triggering an event:
  5. resources/ext.popups.renderer.desktopRenderer/desktopRenderer.js,
  6. resources/ext.popups.desktop/ext.popups.renderer.article.js
  7. resources/ext.popups.desktop/ext.popups.settings.js
  8. resources/ext.popups.targets.desktopTarget/desktopTarget.js

This is problematic as it means our rendering functions have side effects.
I propose that we separate rendering logic from event logging logic.

On inspection it seems like the ext.popups.core or ext.popups.renderer.desktopRenderer would be a good place to keep this logic however ext.popups.renderer.desktopRenderer mixes initialisation of Hovercards and events with rendering

Suggested changes:

  1. Logic in mw.popups.render.render should be moved to a new PopupsGateway class. Hopefully this method can be entirely removed.
  2. The openPopup method should be repurposed as a showPopup method which is passed data from the PopupsGateway class. It should not have to think about caching or know anything about the link that triggered it
  3. In ext.popups.core.js the gateway should be used to retrieve the data for the hovercard and this should be passed to the openPopup method and rendered.
  4. Introduce a PopupsController class that dispatches to render methods and event logging methods when necessary.

Event Timeline

Most of it is obsolete, probably. Needs analysis.