Currently the renderer for Popups mixes view and controller logic.
- Code in ext.popups.core.js calls a render method
- The mw.popups.render.render method makes an api request via a call to mw.popups.render.renderers.article.init
- When this is completed it makes a call to mw.popups.render.openPopup
- The mw.track method is called in [[ URL | various ]] places by the renderer - a render cannot be called without triggering an event:
- resources/ext.popups.renderer.desktopRenderer/desktopRenderer.js,
- resources/ext.popups.desktop/ext.popups.renderer.article.js
- resources/ext.popups.desktop/ext.popups.settings.js
- 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:
- Logic in mw.popups.render.render should be moved to a new PopupsGateway class. Hopefully this method can be entirely removed.
- 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
- 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.
- Introduce a PopupsController class that dispatches to render methods and event logging methods when necessary.