Restore sparse per-component module loading#80
Merged
Conversation
The intended-but-broken sparse-loading scheme had two gaps: 1) Only ImageModal::177 called BootstrapComponentsService::registerComponentAsActive(), so HooksHandler::onParserAfterParse's foreach over getActiveComponents() never iterated tooltip / popover / carousel / modal / etc. — their per-component .fix modules were never queued, JS or CSS. 2) The foreach itself only called $parserOutput->addModuleStyles(), so even for ImageModal the JS half of any module with a 'scripts' entry was unreachable through that path. Close both gaps: - AbstractComponent::parseComponent() now calls registerComponentAsActive() right after opening the nesting controller. All 11 explicit-syntax components (Accordion, Alert, Badge, Button, Card, Carousel, Collapse, Jumbotron, Modal, Popover, Tooltip) inherit this via the single chokepoint. Fetched via MediaWikiServices, matching the service-locator pattern already used by LuaLibrary, CarouselGallery, and ImageModal in the codebase. - HooksHandler::onParserAfterParse foreach now calls both addModuleStyles() and addModules() per module. Style-only modules (modal.fix etc.) treat the addModules call as a no-op; modules with a 'scripts' entry (tooltip.fix, popover.fix, carousel.fix) get their init JS loaded. Net effect: a page with only a tooltip loads only tooltip.fix. A page with nothing loads no BC init modules at all. Sparse loading is restored as originally intended. Verified on a fresh MW 1.39.17 + Vector + BC stack and on a MW 1.43.8 + Chameleon BS4 + BC stack: tooltip and popover both fire; carousel.fix stays mw.loader state 'registered' on pages without a carousel; modal still opens (PR #75's inline-emission fix is orthogonal and unaffected). PHPUnit unit suite green relative to baseline (6 pre-existing failures unchanged). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Asserts that HooksHandler::onParserAfterParse calls both addModuleStyles and addModules on the ParserOutput for each active component's modules. Previously only addModuleStyles was called, so components with a scripts entry (tooltip.fix, popover.fix, carousel.fix) had their init JS dropped. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5e32031 to
e7f2f3f
Compare
This was referenced May 21, 2026
Merged
| $this->assertContains( 'ext.bootstrapComponents.tooltip.fix', $loadedStyles ); | ||
| $this->assertContains( 'ext.bootstrapComponents.popover.fix', $loadedStyles ); | ||
| $this->assertContains( 'ext.bootstrapComponents.tooltip.fix', $loadedModules ); | ||
| $this->assertContains( 'ext.bootstrapComponents.popover.fix', $loadedModules ); |
There was a problem hiding this comment.
I see lots of LOC, mocks, and binding to details for testing one line of production code. Highly sus and likely notably net-negative
malberts
added a commit
that referenced
this pull request
May 22, 2026
`AbstractComponent::augmentParserOutput()` used to load each component's per-skin module variants by calling `getModulesFor()` with a hard-coded `'vector'` skin name, which the JSON registry treats as "always include the vector entry alongside the default". That published the `modal.vector-fix` and `popover.vector-fix` CSS to every skin's output, hiding the `.modal-backdrop` element on Vector-family chromes that would otherwise overlay the page. MediaWiki 1.43's ParserOutputAccess lifecycle change broke that publication path: the modules attached via the augment hook no longer reach the OutputPage. The earlier sparse-loading patch in `HooksHandler::onParserAfterParse` (5.2.3) routed the default modules through `ParserAfterParse` instead, but its foreach didn't pass any skin argument, so the per-skin entries stopped loading on every skin. Users on Vector, Vector 2022, MonoBook, and Timeless all end up with the BS4 modal backdrop covering the page once a modal opens. Pass the same hard-coded `'vector'` into the post-parse foreach so the registry returns the per-skin variants exactly as it did pre-1.43. The redesign of this path (route by active skin name, audit per-skin requirements) belongs in the next major release. Drop the heavy mock-based unit test that landed alongside the original sparse-loading patch. Per the reviewer comment at #80 (review) the test pinned a single argument literal through deep mocks of every collaborator on `HooksHandler`, coupling to call signatures rather than behaviour. Integration verification on the running rig covers the regression more reliably. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
malberts
added a commit
that referenced
this pull request
May 22, 2026
Per the reviewer comment at #80 (review) the test pinned a single argument literal through deep mocks of every collaborator on `HooksHandler`, coupling to call signatures rather than behaviour. Integration verification on the running rig covers the regression more reliably. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
malberts
added a commit
that referenced
this pull request
May 22, 2026
Per the reviewer comment at #80 (review) the test pinned a single argument literal through deep mocks of every collaborator on `HooksHandler`, coupling to call signatures rather than behaviour. Integration verification on the running rig covers the regression more reliably. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
malberts
added a commit
that referenced
this pull request
May 22, 2026
Per the reviewer comment at #80 (review) the test pinned a single argument literal through deep mocks of every collaborator on `HooksHandler`, coupling to call signatures rather than behaviour. Integration verification on the running rig covers the regression more reliably. Cross-port of the equivalent removal on the 5.x maintenance branch. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
malberts
added a commit
that referenced
this pull request
May 22, 2026
Per the reviewer comment at #80 (review) the test pinned a single argument literal through deep mocks of every collaborator on `HooksHandler`, coupling to call signatures rather than behaviour. Integration verification on the running rig covers the regression more reliably. Cross-port of the equivalent removal on the 5.x maintenance branch. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #68.
Restores BC's intended sparse-loading scheme by closing the two gaps that broke it. +5 lines across 2 source files.
Root cause
BC was designed to load each component's
.fixResourceLoader module only on pages that actually use that component — sparse, on-demand loading. The mechanism: each component callsBootstrapComponentsService::registerComponentAsActive($name)when used, andHooksHandler::onParserAfterParseiterates the registered set to queue the per-component modules. Two gaps broke that scheme:ImageModal::177calledregisterComponentAsActive(). The 11 explicit-syntax components (Accordion, Alert, Badge, Button, Card, Carousel, Collapse, Jumbotron, Modal, Popover, Tooltip) never registered themselves, so the foreach never iterated over them — their.fixmodules were never queued, CSS or JS. ImageModal was the only one that worked.$parserOutput->addModuleStyles()(CSS only). Modules with ascripts:entry (tooltip.fix,popover.fix,carousel.fix) never had their JS half loaded —addModuleStylesdoesn't cover that.Fix
Two one-line additions, at the right architectural layer in each case:
AbstractComponent::parseComponent()now callsregisterComponentAsActive($this->getComponentName())right after$nestingController->open($this). All 11 explicit-syntax components inherit this via the single chokepoint. The service is fetched viaMediaWikiServices::getInstance()->getService(...), matching the service-locator pattern already used byLuaLibrary,CarouselGallery, andImageModalin the codebase.HooksHandler::onParserAfterParseforeach now calls bothaddModuleStyles([$module])andaddModules([$module]). Style-only modules treat the latter as a no-op; modules with ascripts:entry get their JS init loaded.Scope
AbstractComponent.php,HooksHandler.php), +5 lines / -0HooksHandlerTest.php), +59 lines / -0 — new unit test pinning the foreachaddModulescontractVerification
Tested on MW 1.43.8 + Chameleon BS4 (where the bug manifests) and on the oldest supported floor MW 1.39.17 + Vector:
TooltipPopoverTestpagetooltip.fix+popover.fix=ready✓;carousel.fix=registered(not loaded — sparse!) ✓ModalTestpagemodal.fix=ready✓; tooltip / popover / carousel.fixall stayregistered(no triggers on this page) ✓.show, backdrop,body.modal-open) ✓TooltipPopoverTestpageTooltipPopoverTestpageTooltipPopoverTestpageVisual confirmation of the interactions:
The sparse-load assertion is the key new evidence: on a tooltip-only page, only
tooltip.fix+popover.fixload. On a modal-only page, onlymodal.fixloads. On a page with nothing, no BC init modules load. Original design intent restored.Tests
New:
HooksHandlerTest::testOnParserAfterParseLoadsActiveComponentScriptsAndStylesasserts that the foreach calls bothaddModuleStylesANDaddModulesper active component'"'"'s module — pins the contract this PR restores against future regressions.PHPUnit (
--group mediawiki-databaseless) on MW 1.39: 339 tests, 537 assertions, 6 failures — baseline plus one new test, no new failures (same 6 pre-existingBootstrapComponentsServiceTest/ImageModalTriggerTest/ImageModalTestfailures, all unrelated to the module-loading path).