Repository navigation
refactor!: move the plugin override registry off Redux onto React context - #2082
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2082 +/- ##
==========================================
+ Coverage 93.67% 93.89% +0.21%
==========================================
Files 365 365
Lines 5886 5895 +9
Branches 1405 1370 -35
==========================================
+ Hits 5514 5535 +21
+ Misses 356 347 -9
+ Partials 16 13 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0c924bd to
856a95d
Compare
856a95d to
16ed9de
Compare
16ed9de to
4e6c582
Compare
4e6c582 to
9eef293
Compare
arbrandes
left a comment
There was a problem hiding this comment.
Approved. The breaking change only requires minor changes to the only existing plugin.
We should discuss refactoring the registry (or doing away with it) for the move to frontend-base. Claude helped me think this through:
The only real registrar is the published @edx/unit-translation-selector-plugin. What it needs:
- Render in the unit title slot, with the slot props and the course language.
- Stay hidden until an async config fetch confirms the feature and its languages.
- Let the learner pick a language mid-session, persisted per course in localStorage.
- Add
src_lang/dest_langto the iframe URL when the pick differs from the course language, without a page reload, on every change. - Keep applying across the
<Unit key={unitId}>remount without a first-render gap.
None of that needs a function pipeline in Learning. We could use slots: the seam would be a unit content slot with ContentIFrame as its default widget. The plugin would REPLACE that widget with its own composition of the exported ContentIFrame and getIFrameUrl, adding src_lang/dest_lang from state it owns (requirement 4). Its selector stays a title-slot widget (1), gated on the config fetch (2), and the selection would live in a provider registered through App.providers (3). A provider mounted above the content widget has the value on its first render, so the reload-or-stale trade-off behind requirement 5 does not arise; and because the plugin's state stays in its own provider as data, neither does the re-render loop from #1330's plugins/README.md caveat.
What that asks of Learning: export ContentIFrame and pass the default iframeUrl on the slot, so the plugin composes rather than reimplements, and treat those as the slot's contract. Because the REPLACE is declared at config time and condition is synchronous, the plugin's widget has to self-gate, rendering the plain default when the feature is off or no language differs.
Generally speaking, frontend-base has no value-filter pipeline and I'm not sure we want one. Plus, ADR 0011's case against Wrap seems to apply to the override registry's function chain too. If the feature can be implemented as an app - even if we need to add a slot for it - it seems to be a way saner proposition.
Claude kept trying to use that as justification for making a more narrow solution in this PR, and I had to push back a few times. The reality is that we don't know how the override functionality is being used by plugins at the moment, so keeping the pre- That being said, I 100% agree that this isn't how we should do this in |
…text
The `generic/plugin-store` slice let a plugin rendered in a slot register
an override for a host-computed value (`registerOverrideMethod`), and the
host folded the registered overrides over its default
(`usePluginsCallback`). It was learning's own Redux, and the only reason
`store.ts` carried a middleware option (the serializable-check exemption,
because the payload is a function).
It is client state — nothing is fetched — so it moves to React context:
`PluginOverridesProvider` holds the registry, mounted at the route root
beside `ToastProvider`. The contract is unchanged: same
`{ pluginName, methodName, method }` payload, same fold (default first,
then each override in registration order), overwrite on re-register, and
overrides outlive the component that registered them. Plugins obtain
`registerOverrideMethod` from `usePluginOverrides()` and call it from
their own effect instead of dispatching it. New: `unregisterOverrideMethod`
for plugins that want cleanup. `usePluginsCallback` keeps its signature,
so the unit component changes only its import path.
`store.ts` drops the `plugins` reducer and the whole `middleware` option,
leaving `models` + `specialExams`. The module gets its first tests; the
`setupTest.js` module mock (which faked an empty registry because the test
store never mounted the reducer) is replaced by the real provider in the
render wrapper, and the unit test gains a case where a registered override
reaches the iframe `src`. `generic/plugin-overrides/README.md` documents
the contract and the migration.
Part of #1946 (Stage 1). Closes #2017.
BREAKING CHANGE: `registerOverrideMethod` is no longer a Redux action
creator and cannot be dispatched. Plugins import `usePluginOverrides` from
`@src/generic/plugin-overrides` (was `@src/generic/plugin-store`) and call
`registerOverrideMethod(payload)` from an effect; see
`src/generic/plugin-overrides/README.md`. The `plugins` reducer is gone
from the store.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
9eef293 to
eefe432
Compare
Summary
Move the plugin override registry off Redux onto React context.
generic/plugin-storelet a plugin rendered in a slot register an override for a host-computed value (registerOverrideMethod), and the host folded the registered overrides over its default (usePluginsCallback); its one in-repo consumer is the unit content iframe URL. It was learning's own Redux and the only reasonstore.tscarried a middleware option. Nothing in it is server state, so it becomes aPluginOverridesProviderwith the same contract — payload, fold order, overwrite, persistence — and only the transport changes.store.tsis nowmodels+specialExams. Part of the Redux → React Query migration (#1946, Stage 1), stacked on #2081. Closes #2017.What changed
generic/plugin-overrides/PluginOverridesContext.tsx(new, replacesgeneric/plugin-store/{slice,hooks,index}.js).PluginOverridesProviderholdspluginName → methodName → methodinuseState;usePluginOverrides()exposesregisterOverrideMethod(same{ pluginName, methodName, method }payload as the old action creator, overwrite on re-register) and a newunregisterOverrideMethod;usePluginsCallback(methodName, defaultMethod)keeps its signature and fold (default first, then each registered override receives the previous result, in registration order). Both hooks throw outside the provider, likeuseToast. Noany: the stored map is typed(previousResult: never) => unknown, which accepts anyOverrideMethod<T>on registration without a cast and is uncallable until the host asserts itsTin the fold — the single cast in the module, at the one place the host knows the type.SequenceContentrenders<Unit key={unitId}>, so the slot widget remounts per unit; for a registrar gated behind an async fetch, cleanup would make the next unit's iframe load un-overridden and then reload. Today's behavior (the previous unit's override covers the first render) is preserved; plugins that want cleanup callunregisterOverrideMethodfrom their effect.src/index.jsxmountsPluginOverridesProvideraroundRoutes, insideToastProvider.src/store.tsdrops thepluginsReducerand the entiremiddlewareoption (the serializable-check exemption existed for this slice alone).Unit/index.jsxchanges only its import path.generic/plugin-overrides/README.md(new): the plugin-author contract (registering, fold semantics, thegetIFrameUrlmethod the host folds today, adding a host method) and a before/after migration note from the Redux version.unregisterremoving exactly one entry, persistence after the registrar unmounts, and the throw outside the provider.setupTest.js's render wrapper providesPluginOverridesProviderand the module mock is deleted (it faked an empty registry because the test store never mounted the reducer), soUnit/index.test.jsx's existing iframe-URL assertion now runs the real fold; it also gains a case where a sibling component registers agetIFrameUrloverride and the iframesrcreflects it.Breaking change
registerOverrideMethodis no longer a Redux action creator and cannot bedispatched; plugins importusePluginOverridesfrom@src/generic/plugin-overrides(was@src/generic/plugin-store) and callregisterOverrideMethod(payload)from an effect. Thepluginsreducer is gone from the store. Same class as #2077. The one known external registrar (@edx/unit-translation-selector-plugin, see the baseline-example comment on #2017) also imports the model store'suseModel, which #1977 will break — the two should ship as one plugin release.Testing
npm run types(0 errors),npm run lint(clean), full jest suite green at head (109 suites, 1112 passed / 3 pre-existing skips). Manual pass on tutor local with the baseline example plugin from the #2017 issue comment ported to the new API: all five by-hand checks passed; the store shape and override persistence rest onstore.tsand the context tests respectively — see the details block below.Decisions
Full decision log
Decisions — plugin override registry off Redux onto React context (#2017)
Entries 1–9 were settled in the plan (issue #2017 body, 2026-09-20) before
implementation; 10 onward landed with the code.
Client state → React context, not React Query. Nothing in the registry
is fetched, cached or invalidated: plugins (descendants) contribute
overrides and the host component (an ancestor) reads them. That is the
context half of OEP-0067 ADR-0010.
PluginOverridesProviderholds theregistry in
useStateand is mounted once at the route root besideToastProvider, the migration's established client-state pattern (statein the provider, use in the consumer).
The generic contract is preserved; only the transport changes. The
registerOverrideMethod({ pluginName, methodName, method })payload, thefold (default first, then each registered override receives the previous
result, in registration order), overwrite on re-register, and persistence
after the registrar unmounts are unchanged. The registry is a public
extension surface — operator
env.config.jsxplugins are untracked andunsearchable — so replacing it with per-behavior typed hooks would break
plugins we can't see. Functions in state were only awkward under Redux's
serializability rule (the
store.tsmiddleware exemption); context has nosuch rule, so keeping the generic shape costs nothing.
Plugins register imperatively from their own effect. Today's
"action creator + dispatch inside the plugin's
useEffect" becomes"setter from context + call inside the plugin's
useEffect". The pluginkeeps ownership of when it registers (its deps). An effect-owning
convenience hook can be added later without breaking this; not in this PR.
No implicit cleanup on unmount; additive
unregisterOverrideMethod.SequenceContentrenders<Unit key={unitId}>, soUnitand the slotwidget remount on every unit change. For a registrar gated behind an async
fetch (the known external plugin renders nothing until its config fetch
resolves), implicit cleanup would mean unit B's first render has no
override → iframe loads un-overridden → fetch resolves → register → URL
changes → iframe reloads. Today unit A's override covers B's first render
and the iframe loads once. So the provider does not clean up on its own;
it exposes
unregisterOverrideMethod({ pluginName, methodName })forplugins that want cleanup from their effect.
Names drop the Redux vocabulary; plugin-facing verbs stay.
generic/plugin-store→generic/plugin-overrides("store" is the wordbeing removed; the break is happening anyway so the import-path change
rides in the same major).
PluginOverridesProvider/usePluginOverrides().registerOverrideMethodkeeps its name — it describes what the plugindoes, not the transport. Host-side
usePluginsCallback(methodName, defaultMethod)keeps its name and signature;Unit/index.jsxchanges onlyits import path.
Throw outside the provider, matching
useToast. The provider ismounted at the root in the app and in
setupTest'srenderwrapper.store.tsloses its only middleware option along with the reducer.The
serializableCheckexemption existed for this slice alone; the storeis now
configureStore({ reducer: { models, specialExams } }).BREAKING (
refactor!:+BREAKING CHANGE:footer), same class asrefactor!: convert the progress-tab exam attempts fetch to a React Query hook #2077.
registerOverrideMethodcan no longer bedispatched and theimport path moves. A
generic/plugin-overrides/README.mdcarries theplugin-author contract and the before/after migration note. The one known
external registrar (
@edx/unit-translation-selector-plugin) also importsthe model store's
useModel, which Dissolve the model-store normalized cache #1977 breaks — the two should ship asone plugin release.
neverin the stored map, one cast in the fold — noany. Themethod-name set is open (decision 2) and each name's value type is the
host's business, so the map can't name them. Two ways to say "unknown per
key":
any(instantly readable, twono-explicit-anydisables, butunsound in both directions and silently — the fold would return
anyintothe
Taccumulator with no visible assertion) or a parameter ofnever:Record<string, Record<string, (previousResult: never) => unknown>>. Afunction taking
stringis assignable to one takingnever, so anyOverrideMethod<T>registers without a cast (including from a typedplugin), and the stored method is uncallable until the host asserts its
T—(plugin[methodName] as OverrideMethod<T>)(result)inusePluginsCallback. That forces the single trust point onto the page atthe one place the host knows
T, needs no lint disables, and doesn'tpropagate the way
anydoes. Chosen overanyfor that visibility; thereadability cost is paid once by the comment on the type. JS plugins
(
env.config.jsx, compiled npm packages) are unaffected: types are erasedand
tscnever includes root.jsxfiles.Tests run the real registry for the first time. The module had no
tests, and
setupTest.jsmockedusePluginsCallbackbecause the teststore never mounted the
pluginsreducer (the real hook would have thrownon
Object.values(undefined)). The testrenderwrapper now providesPluginOverridesProvider, the module mock is deleted, andUnit/index.test.jsx's existing iframe-URL assertion therefore exercisesthe real (empty) fold. New
PluginOverridesContext.test.tsxpins:default-only, composition order across two plugins, overwrite, skip of
other method names, per-call default evaluation,
unregisterremovingexactly one entry, persistence after the registrar unmounts (decision 4),
and the throw outside the provider.
Unit/index.test.jsxgains aconsumer-level case: a sibling component registers a
getIFrameUrloverride and the iframe
srcreflects it.Generic functions avoid shapes the autofixer breaks. A generic arrow
(
<T,>(…) =>) in a.tsxfile is autofixed by the repo's ESLint into<T>(…), which the parser then reads as JSX and fails on; a namedgeneric function expression inside
useCallbackgets rewritten to anarrow by
prefer-arrow-callbackand loses its<T>(and because Babelstrips types, tests still pass — only
tsccatches it, so runlint:fixbefore
types). HenceusePluginsCallbackis afunctiondeclaration, and
registerOverrideMethodis typed throughuseCallback<PluginOverridesContextValue['registerOverrideMethod']>(…),letting the arrow's parameter pick up
Tcontextually.Manual testing
Checklist
Manual testing — plugin override registry onto React context (#2017)
In-browser verification against a live backend (tutor local). This layer
claims no user-visible change for a learner: with no plugin registered the
unit iframe URL is exactly what
getIFrameUrlbuilds, and with a pluginregistered the override applies the same way it did through Redux. The
observable changes are for plugin authors (the registration API) and in the
store (no
pluginsslice).Setup (DemoX on tutor local)
Course id:
course-v1:OpenedX+DemoX+DemoCourse; any unit under/course/course-v1:OpenedX+DemoX+DemoCourse/....API, lives in the repo-root
env.config.jsx(gitignored). It inserts aDIRECT_PLUGINintoorg.openedx.frontend.learning.unit_title.v1thatrenders a Paragon Card with a switch; the switch registers a
getIFrameUrloverride that sets
show_title=1(thegetIFrameUrldefault is0) or apass-through.
nvm use && npm run dev; the dev server picks upenv.config.jsxat therepo root automatically (frontend-build resolves
env.config).#unit-iframeelement'ssrcattribute indevtools; "the in-iframe title" means the unit title the LMS renders inside
the xblock content when
show_title=1.Verify by hand
"Plugin override example" card appears above the unit title, the
default title and bookmark button still render (
keepDefault), noconsole error from
usePluginOverrides.#unit-iframesrchasshow_title=0; no in-iframe title.srcchanges toshow_title=1andthe in-iframe title appears (the title now shows twice: MFE + LMS).
srcback toshow_title=0, in-iframe title gone. Toggle a few times; each fliptakes effect (latest registration wins, no stacking).
env.config.jsx,restart dev: a unit renders with the default title row and the iframe
srchasshow_title=0; no errors (the provider is mounted with anempty registry).
Left to the automated suite (not re-done by hand)
pluginsslice — not a runtime observation:the reducer map is
src/store.ts(models+specialExams),tscchecks
RootStateagainst every remaining reader, andgit grep 'state\.plugins'oversrcis empty.PluginOverridesContext.test.tsx: default-only,composition order across two plugins, overwrite, skipping other method
names, per-call default evaluation,
unregisterOverrideMethodremovingexactly one entry, persistence after the registrar unmounts, throw outside
the provider.
src—Unit/index.test.jsx(
applies a registered override to the iframe src); the existinggenerates correct iframeUrlcase now runs the real (empty) fold insteadof the deleted
setupTestmock.plugin); rests on the composition-order unit test.
PluginOverridesContext.test.tsx(keeps an override after the component that registered it unmounts). A by-hand version was tried and dropped: theexample widget keeps its switch in
useState, so on the<Unit key={unitId}>remount it comes back off and re-registers the pass-through, overwriting the
persisted override — the same thing the Redux slice would do. That tests the
example's state handling, not the registry. A plugin that re-registers the
same transform after a remount (the translations plugin keeps its language
in
localStorage) gets the first-render coverage decision 4 describes.Results
Env: tutor local, DemoX, local branch
bsmith/plugin-overrides-context@0c924bda(no PR yet). Run 2026-09-20.out, dev server restarted).
by-hand list: after navigating to the next unit the iframe
srcwas back toshow_title=0, because the example's switch state reset on the remount andit re-registered the pass-through. That is the example overwriting its own
entry, which the Redux slice would also have done; registry persistence is
covered by the unit test named above.
pluginsslice in the store — not run by hand (static property ofstore.ts; see above).🤖 Generated with Claude Code