Merge pull request 'fix(modules): core offers a contribution, never a slot name' (#160) from feature/teams-slot-contributions into edge

Reviewed-on: #160

client/src/main.jsx

@@ -3,7 +3,7 @@ import { createRoot } from 'react-dom/client'
 import { BrowserRouter } from 'react-router-dom'
 import App from './App.jsx'
 import { publishSharedDependencies } from './modules/shared.js'
-import { declareSlot, applyCoreFills, fillModuleSlot } from './modules/registry.js'
+import { declareSlot, applyCoreFills, offerCoreFill } from './modules/registry.js'
 import TeamActivityFeed from './modules/TeamActivityFeed.jsx'
 import TeamForumPanel from './modules/TeamForumPanel.jsx'
 import TeamNotifyToggle from './modules/TeamNotifyToggle.jsx'
@@ -72,24 +72,33 @@ declareSlot('player.invite.accepted')
 // module chunk has evaluated, which is the only moment a module-declared slot
 // exists to be filled.
 //
-// Naming a slot no installed module declares is not an error. On a deployment
-// with no game module this fill simply never lands, which is the mirror of an
+// **Core offers a CONTRIBUTION and never names a slot.** The module that owns the
+// page says where each of these goes, in its own vocabulary, by asking for one on
+// `declareModuleSlot`. Naming the slots here instead — which is how this was first
+// written — meant core's Team content reached exactly one module: any other game
+// declaring a place under its own id got an empty page and no error, because a
+// fill nobody asked for is deliberately not an error. It also put a module id
+// inside core, in string literals `scripts/checkModuleIdentifiers.js` masks by
+// construction and so could never have caught.
+//
+// Offering something nothing asks for is still not an error: a deployment with no
+// game module installed asks for none of these, which is the mirror of an
 // unfilled slot rendering nothing.
-fillModuleSlot('uo.guild.detail', TeamActivityFeed)
+offerCoreFill('team.activity', TeamActivityFeed)
 
-// The forum is core's for the same reason and goes in a SECOND place the module
-// declares, rather than joining the feed in the first: a slot takes one component
-// (first fill wins), and stacking two unrelated panels into one fill would make
-// the module unable to place them separately on its own page. It also keeps the
-// two independent — a deployment with the forum switched off renders the feed
-// exactly as before.
-fillModuleSlot('uo.guild.forum', TeamForumPanel)
+// The forum is core's for the same reason and goes wherever the module asked for
+// it — a SECOND place, in module-uo's case, rather than joining the feed in the
+// first: a slot takes one component (first fill wins), and stacking two unrelated
+// panels into one contribution would make the module unable to place them
+// separately on its own page. It also keeps the two independent — a deployment
+// with the forum switched off renders the feed exactly as before.
+offerCoreFill('team.forum', TeamForumPanel)
 
-// And the notification control, in a third place the module declares ABOVE its
-// roster. A third slot rather than a corner of the feed for the same reason there
-// were two: this is an action on the page and the other two are content in it,
-// and only the module can say where each belongs on a page it owns.
-fillModuleSlot('uo.guild.header', TeamNotifyToggle)
+// And the notification control. A third contribution rather than a corner of the
+// feed for the same reason there were two: this is an action on the page and the
+// other two are content in it, and only the module can say where each belongs on
+// a page it owns.
+offerCoreFill('team.notify', TeamNotifyToggle)
 
 // Render on DOMContentLoaded rather than immediately, and that is the one line
 // of core's boot the module system changes.

client/src/modules/registry.js

@@ -135,6 +135,37 @@ export function declareSlot(name) {
   slots.set(name, { Component: null, filledBy: null })
 }
 
+/**
+ * The contributions core has for a module-declared slot.
+ *
+ * **Core offers a CONTRIBUTION, not a slot name, and that is the whole of why
+ * this list exists.** The first cut of the inverted direction had core fill three
+ * literal names — `uo.guild.detail` and its two siblings — which worked for
+ * exactly one module and silently did nothing for any other: a second game
+ * declaring `clan.detail` under its own id got an empty page and no error,
+ * because "a fill for a slot nobody declared is not an error" is the rule that
+ * makes an unknown name invisible. It also put a module identifier in core, in
+ * three string literals `scripts/checkModuleIdentifiers.js` cannot see, since it
+ * masks string bodies by construction.
+ *
+ * So the module says WHERE (its own slot, in its own vocabulary) and WHICH of
+ * core's contributions goes there. Core never names a module id.
+ *
+ * Adding a member here is a **minor** MODULE_API bump. Requesting one that is not
+ * here THROWS at the declaration, deliberately: unlike an unfilled slot, an
+ * unknown contribution is always a typo or a version skew — core's list is fixed
+ * at build time and a module's `coreApi` range has already been checked — and the
+ * failure it would otherwise produce is a page that renders empty forever.
+ */
+export const CORE_CONTRIBUTIONS = Object.freeze({
+  /** The Team activity feed. Core's because only core can resolve the public/members split on it. */
+  'team.activity': true,
+  /** The Team forum panel. Core's because membership and manual grants are core's rules. */
+  'team.forum': true,
+  /** The per-Team notification control. Core's because it resolves whether the viewer is in the Team. */
+  'team.notify': true,
+})
+
 /**
  * The INVERTED direction: a MODULE declares a slot and CORE fills it.
  *
@@ -153,48 +184,68 @@ export function declareSlot(name) {
  * what stops two modules colliding and what makes the owner readable at the fill
  * site. The namespace is enforced rather than conventional.
  *
+ * **`options.core` names which of core's contributions belongs in that place.**
+ * It is optional — a module may declare a slot it fills itself, or one it keeps
+ * empty for now — and it is the only thing that gets core's content into the
+ * page. The place name stays the module's own word; the contribution is core's.
+ *
  * **Ordering is why this is a separate call and not just `declareSlot` exposed
  * to modules.** Core's bundle evaluates BEFORE any module chunk (module scripts
  * are deferred and injected after core's), so at the moment core would like to
- * fill one of these, it does not exist yet. Core therefore registers its fills
- * through `fillModuleSlot` below, which is applied after every module chunk has
- * evaluated — see main.jsx.
+ * fill one of these, it does not exist yet. Core therefore offers its
+ * contributions through `offerCoreFill` below, applied after every module chunk
+ * has evaluated — see main.jsx.
  */
-export function declareModuleSlot(id, name) {
+export function declareModuleSlot(id, name, options = {}) {
   if (!name.startsWith(`${id}.`)) {
     throw new Error(`declareModuleSlot: "${name}" must be namespaced "${id}."`)
   }
   if (slots.has(name)) throw new Error(`extension slot "${name}" already declared`)
-  slots.set(name, { Component: null, filledBy: null, declaredBy: id })
+  const contribution = options.core ?? null
+  if (contribution !== null && !Object.hasOwn(CORE_CONTRIBUTIONS, contribution)) {
+    throw new Error(
+      `declareModuleSlot: "${name}" asks for core contribution "${contribution}", which core does not ` +
+      `offer. Known: ${Object.keys(CORE_CONTRIBUTIONS).join(', ')}.`,
+    )
+  }
+  slots.set(name, { Component: null, filledBy: null, declaredBy: id, wants: contribution })
 }
 
-// Core's pending fills for module-declared slots, applied once every module
-// chunk has evaluated. Kept as a list rather than applied eagerly because the
-// slot does not exist when core asks — see the ordering note above.
+// Core's pending contributions, applied once every module chunk has evaluated.
+// Kept as a list rather than applied eagerly because no module-declared slot
+// exists when core offers — see the ordering note above.
 const coreFills = []
 
 /**
- * Core: "fill this module-declared slot when it turns up."
+ * Core: "here is my <contribution>, for whichever module asked for it."
  *
- * Deliberately not an error when the slot never appears. A module that is not
- * installed declares nothing, and core offering content for a page that does not
- * exist is the ordinary case on any deployment — not a misconfiguration. That is
- * the mirror of an unfilled slot rendering nothing.
+ * Deliberately not an error when nothing asked. A deployment with no game module
+ * installed asks for none of these, and core offering content for a page that
+ * does not exist is the ordinary case rather than a misconfiguration — the mirror
+ * of an unfilled slot rendering nothing.
+ *
+ * More than one slot may ask for the same contribution, and each gets it. Core
+ * has no reason to care how many places a module wants its feed in, and refusing
+ * the second would be core making a layout decision on a page it does not own.
  */
-export function fillModuleSlot(name, Component) {
-  if (typeof Component !== 'function') throw new Error(`fillModuleSlot: ${name} is not a component`)
-  coreFills.push([name, Component])
+export function offerCoreFill(contribution, Component) {
+  if (!Object.hasOwn(CORE_CONTRIBUTIONS, contribution)) {
+    throw new Error(`offerCoreFill: "${contribution}" is not in CORE_CONTRIBUTIONS`)
+  }
+  if (typeof Component !== 'function') throw new Error(`offerCoreFill: ${contribution} is not a component`)
+  coreFills.push([contribution, Component])
 }
 
-/** Apply core's fills. Called once from main.jsx, after module chunks have run. */
+/** Apply core's contributions. Called once from main.jsx, after module chunks have run. */
 export function applyCoreFills() {
-  for (const [name, Component] of coreFills) {
-    const entry = slots.get(name)
-    if (!entry) continue // the declaring module is not installed
+  for (const [contribution, Component] of coreFills) {
+    for (const entry of slots.values()) {
+      if (entry.wants !== contribution) continue
       if (entry.filledBy) continue // a module already claimed it; first fill wins
       entry.Component = Component
       entry.filledBy = 'core'
     }
+  }
   coreFills.length = 0
 }
 

client/src/modules/shared.js

@@ -67,7 +67,7 @@ const ui = {
   useAsync,
   useAuth,
   useSite,
-  // The eighth member, for the INVERTED slot direction (TEAMS.md Part 3). A
+  // The ninth member, for the INVERTED slot direction (TEAMS.md Part 3). A
   // module that declares a slot on its own page needs the same component core
   // renders its own with — the error boundary in particular, since the thing
   // being contained here is CORE's content failing inside the MODULE's page.

client/test/moduleSlots.test.js

@@ -5,7 +5,8 @@ import {
   registry,
   declareSlot,
   declareModuleSlot,
-  fillModuleSlot,
+  offerCoreFill,
+  CORE_CONTRIBUTIONS,
   applyCoreFills,
   registerExtension,
   extensionFor,
@@ -112,34 +113,72 @@ test('a module-declared slot must be namespaced under the declaring module', ()
   assert.doesNotThrow(() => declareModuleSlot('uo', 'uo.guild.detail'))
 })
 
-test('core fills a module slot only after the module has declared it', () => {
-  // The ordering that makes this a separate call: core's bundle evaluates BEFORE
-  // any module chunk, so at the moment core registers its fill the slot does not
-  // exist yet. Filling eagerly would silently do nothing.
-  fillModuleSlot('uo.guild.detail', Feed)
-  assert.equal(extensionFor('uo.guild.detail'), null, 'not filled before the module declared it')
-
-  declareModuleSlot('uo', 'uo.guild.detail')
-  assert.equal(extensionFor('uo.guild.detail'), null, 'and not before the fills are applied')
+test('core offers a contribution and the module says where it goes', () => {
+  // The ordering that makes this two calls: core's bundle evaluates BEFORE any
+  // module chunk, so at the moment core offers, no module-declared slot exists.
+  offerCoreFill('team.activity', Feed)
+  declareModuleSlot('uo', 'uo.guild.detail', { core: 'team.activity' })
+  assert.equal(extensionFor('uo.guild.detail'), null, 'not before the fills are applied')
 
   applyCoreFills()
   assert.equal(extensionFor('uo.guild.detail'), Feed)
 })
 
-test('a fill for a slot nobody declared is not an error', () => {
-  // The module is not installed. Core offering content for a page that does not
-  // exist is the ordinary case on any deployment, not a misconfiguration — the
-  // mirror of an unfilled slot rendering nothing.
-  fillModuleSlot('rust.clan.detail', Feed)
+test('core names no slot, so a second game gets the same content in its own words', () => {
+  // The defect this replaced: core used to fill three literal `uo.guild.*` names,
+  // which reached exactly one module. Every other game declared a place under its
+  // own id and got an empty page with no error, because a fill nobody declared is
+  // deliberately not an error — the rule that makes an unknown name invisible.
+  offerCoreFill('team.activity', Feed)
+  declareModuleSlot('examplegame', 'examplegame.clan.detail', { core: 'team.activity' })
+  applyCoreFills()
+  assert.equal(extensionFor('examplegame.clan.detail'), Feed)
+})
+
+test('two modules can ask for the same contribution, and both get it', () => {
+  // Core has no reason to care how many places want its feed, and refusing the
+  // second would be core making a layout decision on a page it does not own.
+  offerCoreFill('team.activity', Feed)
+  declareModuleSlot('uo', 'uo.guild.detail', { core: 'team.activity' })
+  declareModuleSlot('uo', 'uo.guild.summary', { core: 'team.activity' })
+  applyCoreFills()
+  assert.equal(extensionFor('uo.guild.detail'), Feed)
+  assert.equal(extensionFor('uo.guild.summary'), Feed)
+})
+
+test('a slot that asks for nothing stays empty', () => {
+  // Optional on purpose: a module may declare a place it fills itself, or one it
+  // is keeping for later. Neither is core's business.
+  offerCoreFill('team.activity', Feed)
+  declareModuleSlot('uo', 'uo.guild.detail')
+  applyCoreFills()
+  assert.equal(extensionFor('uo.guild.detail'), null)
+})
+
+test('asking for a contribution core does not offer THROWS', () => {
+  // The asymmetry with an unfilled slot, and it is deliberate. An unknown
+  // contribution is always a typo or a version skew — core's list is fixed at
+  // build time and the module's coreApi range has already been checked — and the
+  // alternative failure is a page that renders empty forever with nothing logged.
+  assert.throws(
+    () => declareModuleSlot('uo', 'uo.guild.detail', { core: 'team.activityfeed' }),
+    /does not offer/,
+  )
+  assert.ok(CORE_CONTRIBUTIONS['team.activity'], 'the catalogue is exported so a test can name it')
+})
+
+test('a contribution nothing asks for is not an error', () => {
+  // No game module installed. Core offering content for a page that does not
+  // exist is the ordinary case on any deployment, not a misconfiguration.
+  offerCoreFill('team.forum', Feed)
   assert.doesNotThrow(() => applyCoreFills())
-  assert.equal(extensionFor('rust.clan.detail'), null)
 })
 
 test('a module that fills its own slot first keeps it', () => {
   const Own = () => null
-  declareModuleSlot('uo', 'uo.guild.detail')
+  declareModuleSlot('uo', 'uo.guild.detail', { core: 'team.activity' })
   registerExtension('uo', 'uo.guild.detail', Own)
-  fillModuleSlot('uo.guild.detail', Feed)
+  offerCoreFill('team.activity', Feed)
   applyCoreFills()
   assert.equal(extensionFor('uo.guild.detail'), Own, 'first fill wins, as everywhere else')
 })
@@ -150,21 +189,21 @@ test('a module-declared slot cannot be declared twice', () => {
 })
 
 test('applying the fills twice does not re-fill or throw', () => {
-  declareModuleSlot('uo', 'uo.guild.detail')
-  fillModuleSlot('uo.guild.detail', Feed)
+  declareModuleSlot('uo', 'uo.guild.detail', { core: 'team.activity' })
+  offerCoreFill('team.activity', Feed)
   applyCoreFills()
   assert.doesNotThrow(() => applyCoreFills())
   assert.equal(extensionFor('uo.guild.detail'), Feed)
 })
 
-test('a non-component fill is refused at the call site, not at render', () => {
-  assert.throws(() => fillModuleSlot('uo.guild.detail', 'nope'), /is not a component/)
+test('a non-component contribution is refused at the call site, not at render', () => {
+  assert.throws(() => offerCoreFill('team.activity', 'nope'), /is not a component/)
 })
 
 test('_reset clears pending fills, so one test cannot leak into the next', () => {
-  fillModuleSlot('uo.guild.detail', Feed)
+  offerCoreFill('team.activity', Feed)
   _reset()
-  declareModuleSlot('uo', 'uo.guild.detail')
+  declareModuleSlot('uo', 'uo.guild.detail', { core: 'team.activity' })
   applyCoreFills()
   assert.equal(extensionFor('uo.guild.detail'), null)
 })