# `src/js` — review, fixes and simplification plan
A full read of the ~16 000 lines under `src/js`, the bugs it turned up, and where the
design could be pared back. Everything in the **Fixed** section is already applied and
covered by tests; everything in **Simplification** is a proposal, not a change.
---
## 1. What the library is
Sixteen thousand lines, no build-time module system: every file calls `goog.provide` and
hangs its exports off one global `MMO` namespace, which Google Closure links into a single
bundle. There is no framework underneath it — the reactive layer, the templating, the
schema validator and the gesture recogniser are all written here.
Five layers, each built on the one below:
```
elements/ CustomElement · CustomShadowElement · CustomAPIElement · Widget · View · Entity
└─ the public surface: custom elements and .jhs views
core/ Map · Template · APIRequester · Printer · Window · Date · Color · File ·
HTMLSearch · URLSearchParams · history · Storage
└─ the reactive model, the dom builder, the server client
schemas/ Schema · Annotation · schemas
└─ a JSON-Schema draft 2020-12 implementation, used to coerce and validate
inputs/ Input · Mouse · Touch · Pointer · TouchMouse · Gesture · Press · Drag
└─ pointer normalisation and gesture recognition
core/functions/ core · dom · strings · path · server · svg · markdown · transitions
└─ leaf utilities, no state
```
### The proxy data model
`MMO.core.Map` extends the native `Map` with change notification, and `MMO.elements.Entity`
extends it again with server identity. It is the spine of the whole library and it is
genuinely good design. Four ideas carry it:
**Values become Maps as they go in.** `set` runs any plain object or array through
`From`, so a tree of data is a tree of observable Maps with no opt-in from the caller.
The rest of the model can then assume every container notifies.
**Children know their parents, plurally.** `parents` is a `Map`, not a single
reference, so the same entity can sit in three collections at once and each parent still
learns about a change. Events walk up every edge, with a `WeakSet` guard for cycles.
**Notification is asynchronous and de-duplicated.** Changes queue on
`MMO.core.Window.run`, which drops a `(callback, args)` pair already queued, and drains on
one animation frame. A hundred writes in a loop repaint once. `emitAsyncLock` /
`emitAsyncUnlock` bracket the compound operations (`clear`, `setValue`, `delete` on an
array) so an observer never sees a half-applied state.
The exception is deliberate and correct: `MAP_CONNECT` / `MAP_DISCONNECT` fire
*synchronously*, because a watch on `a/b/c` has to attach to the newly created `b` before
the queued change on `c` is delivered.
**Watches survive the structure they watch.** `watch('a/b/c')` on a Map where `a` does not
exist yet does not fail — it records the watch as disconnected and listens for
`MAP_CONNECT`. Destroy the branch and rebuild it and the watch reattaches itself. This is
what lets a view bind to a path that the server has not filled in yet, and it is the part
of the design that is hardest to get right.
`Entity` then adds one idea: an **embedded** entity — nested, with no `id` of its own —
keeps a `parent`/`parentKey` link and delegates `save()` upward, so an address saves with
its user and never on its own.
### The view layer
`View extends Template` and binds it to a `Map`. A view has a `contentRoot` and a
`basepath`; `$text`, `$attr`, `$html`, `loop` and `test` each open a watch and rewrite the
dom when it fires. Two details are worth knowing:
- a view **follows its own key**. Rename `a` to `b` in the model and the view rewrites its
`basepath` and emits `path`; delete the value and the view removes itself. A `loop` is
built entirely on this: it does not diff, it lets each row view react to its own key.
- `$$` and `$#` at the end of a path address the **key** and the **index** rather than the
value, so a template can bind to, and write, a row's position.
---
## 2. Fixed
31 defects, each now covered by a test. Ordered by how much damage they do.
### Data corruption and hangs
| Where | What |
|---|---|
| `core/Map.js` `set` | `if (previous === value)` also matched *"key absent, value undefined"*, so `set(k, undefined)` could never insert. `length = n` grows by `push(undefined)` — **it looped forever**. Now `(previous === value) && this.has(key)`. |
| `core/Map.js` `unshift` | `old.parents.set(this, key)` — `key` is not in scope. Every child shifted by an `unshift` recorded `undefined` as its key, so its change events stopped reaching the parent. |
| `core/Map.js` `emitIndexChange` | emitted `MAP_CHANGED` with `(map)` instead of `(type, map)`, unlike every other emit, so listeners read the map as the type. |
| `schemas/Schema.js` `_setContent` | `(Object.hasOwn(data, key)) && (!!data[key])` silently **dropped every falsy keyword**: `const: null`, `const: 0`, `minimum: 0`, `items: false`… `{const: null}` validated `0` as valid. |
| `schemas/Schema.js` `$defs` | definitions were filled in declaration order, so `$defs.item` referencing `#/$defs/sub-item` threw when `sub-item` came later. Now two passes: instantiate every definition, then fill them. |
| `core/EventEmitter.js` `emit` | a listener that threw left `binds.index` set, and **every later emit of that event was silently ignored** as a recursion. Now in a `finally`. |
| `core/functions/dom.js` `loopInputs` | `willValidate` is false for a *disabled* control, and `readonly(node, true)` disables checkboxes — so `readonly(node, false)` could never find them again. A one-way door on any form with a read-only mode. |
### Wrong results
| Where | What |
|---|---|
| `core/functions/core.js` `isSameArray` | `for (let value in a)` iterates indexes, not values: `isSameArray(['a','b'], ['0','1'])` returned true. |
| `core/functions/core.js` `merge` | `[Math.max(a.length, b.length)]` builds a one-slot array *holding the length*. `merge([], [])` returned `[0]`. |
| `core/functions/path.js` `simplify` | `result[result.length]` is always `undefined` (off by one), so a `..` following a `..` was mis-tracked. |
| `core/functions/path.js` `dirname` | tested `i === false`; `lastIndexOf` returns `-1`. `dirname('/a')` returned `''` instead of `'/'`. |
| `core/functions/strings.js` `bytesize` | clamped the unit index to `units.length` instead of `length - 1`: anything past a terabyte rendered as `"0.89 undefined"`. |
| `core/functions/strings.js` `serialize` | pushed a literal `'i: '` instead of the key name, and appended to `hashValues` *after* recursing, so the circular-reference table drifted out of alignment with `hash`. |
| `core/functions/server.js` `updateQuery` | `url += '#' . hash` — that is php. In js it reads a property off a string, so every url with a fragment got `"undefined"` appended. |
| `core/functions/server.js` `extractLink` | `data` and `m` were assigned without declaration (implicit globals, and a `ReferenceError` under strict mode). The shared global regex is now reset before use. |
| `core/functions/server.js` `encodeQuery` | `typeof null === 'object'`, and `Object.entries(null)` throws. |
| `core/functions/dom.js` `hasAttributes` | `for (const name of attrs)` over a plain object throws *"not iterable"*, so `isWrapped(node, tag, attrs)` and `toggleRangeTextWrapping` failed whenever attributes were passed. |
| `core/functions/dom.js` `getEmValue` | arguments to `decodeUnit` were swapped, and `getPropertyValue` was given `'fontSize'` where it needs `'font-size'`. Every `em` conversion was wrong. |
| `core/functions/dom.js` `decodeUnit` | `value * em \|\| getEmValue(node)` parses as `(value*em) \|\| …`, so `0em` silently became the whole font size. |
| `core/functions/dom.js` `encodeUnit` | the `%` branch divided by an undeclared `percent` → `ReferenceError`. Now a parameter. |
| `core/functions/dom.js` `loopAppendChildren` | the `HTMLTemplateElement` branch sat *after* `instanceof Node`, which a template also satisfies — so a `` was appended whole and its content never rendered. |
| `core/URLSearchParams.js` `deleteAll` | `key.length === l` is always true, so `deleteAll('a')` also deleted `ab` and `abc[x]`. |
| `core/Color.js` `From` | `toPrecision()` returns a **string**, so `color.a` was sometimes `'0.50'` — and `a === 1.0` silently failed in `hex()` and `FromStyle`. |
| `core/Date.js` `From` | `this.isTime = true` inside a `static` sets the flag on the *class*, not the date. |
| `core/Date.js` `modify`/`get` | called `setSecondes`/`getSecondes` — neither exists. |
| `core/Date.js` `endOfWeek` | ignored its `firstDayOfWeek` argument entirely. |
| `core/Date.js` `RFC_TIME` | `'(\.[0-9]+)?'` in a single-quoted string is `.`, not an escaped dot: the millisecond group matched any character. |
| `core/APIRequester.js` `fetch` | `(method ?? 'GET' === 'GET')` — `===` binds tighter than `??`, so it evaluated to `method ?? true`. A `PUT` with no body took the GET path. |
| `core/APIRequester.js` `get` | called `addFilter()`, which is commented out, and restored `this.url` by assignment, leaving `searchParams` bound to the old URL. |
| `elements/View.js` `delete` | `path.pop()` on the *string* argument — `TypeError` every time a view deleted itself. Should be, and now is, the array. |
| `elements/View.js` `has` | returned true for any *walkable* path, not an existing one, contradicting its own doc. |
| `elements/View.js` `loop` | the error branch referenced an undefined `key`; `for (v of views)` declared no `v`. |
| `elements/View.js` `htmlUpdater` | `this.nodes.splice(i, 1)` used the `children` index where it needed the `this.nodes` one, removing the wrong node from the tracking list. |
| `elements/CustomAPIElement.js` | `value['collection'] === 'undefined'` compares to the *string*; the guard never fired. |
| `core/history.js` | called `MMO.log` (does not exist) and `api.updateEntity`/`insertEntity`/`removeEntities` (renamed to `update`/`insert`/`remove` long ago). `InsertAction.undo` bailed out with `if (this.executed)` — precisely when it had work to do. |
| `inputs/Gesture.js` `toString` | `'gesture ' + name` reads the global `name`, i.e. `window.name`. |
| `core/Storage.js` | had **no `goog.provide`**: not in the dependency graph, so its load order was luck. |
| `Error.js` | `MMO.ReferenceError` was assigned but never `goog.provide`d. `CustomError` deleted `cause` from the caller's own object. |
### Robustness
`core/Window.js` — `document.currentScript` is null for a deferred, module or injected
script and was dereferenced unguarded; the vendor-prefix detection crashed on a browser
exposing no prefix; `device` and `createAnimationFrame` reached for the ambient globals
instead of `this.window`.
`elements/Widget.js` — called `MMO.fetchEntities` and `super.BuildSchema()`, neither of
which exists, and a `pagination` object with a `getIndexUrl()` that no longer has a class.
It has no callers anywhere in the repo. Rewritten against `MMO.fetchAPI` and the current
`page[offset]`/`page[limit]` parameters.
`core/Printer.js` and `core/Template.js` — `Shared()` and `RenderTemplate()` hard-coded
their own class, so a subclass got a base instance back. Now `new this()`.
### Renamed, with the old name kept
- `MMO.math.HCF` computed the **least common multiple**, not the highest common factor.
`MMO.math.LCM` is the new name; `HCF` stays as a deprecated alias.
- `MMO.core.Map.ketAtIndex` → `keyAtIndex`, old spelling kept as an alias.
### Left alone, on purpose
`MMO.core.Date.compare(date, op)` reads as **`date op this`**, the reverse of what the name
suggests: `a.compare(b, '<')` is true when *b* is before *a*. `isWithin` depends on exactly
that, and callers outside this package may too. Documented loudly rather than flipped.
`core/Date.js` monkey-patches `Date.prototype.getFullYear`, `getMonth`, `getDate`,
`getHours`, `getMinutes` and `getSeconds` **globally, on Firefox**, routing each through
`toLocaleString('fr-FR')`. This is a large hammer: it is orders of magnitude slower than
the native accessors, it forces a French locale on every `Date` in the page including
third-party code, and its own `@TODO` admits `getDay()` is still wrong. Replacing it needs
a reproduction of the Firefox bug it was written for, which is beyond a review — flagging
it as the highest-value thing left to revisit.
---
## 3. Tests
`tests/js` had 4 files, one of which (`core/Map.js`) was **entirely commented out**. It now
has 13, 356 assertions, all passing.
| | |
|---|---|
| `core/Map.js` | 53 — conversion, paths, events, watches, array ops, ordering, parents |
| `core/dom.js` | 39 — creation, attributes, children, ranges, styles, units |
| `elements/View.js` | 34 — content, binding, sub views, loops, tests, watches |
| `core/Date.js` | 27 — parsing, formatting, arithmetic, calendar helpers |
| `core/functions.js` | 27 — equality, merge, watch/link, comparison |
| `elements/Entity.js` | 24 — identity, links, embedding, status, errors |
| `core/Template.js` | 23 — building, attributes, lifecycle, renderers |
| `core/strings.js` | 20 — formatting, regex builders, printf, distances |
| `core/server.js` | 19 — links, queries, urls, requests |
| `core/EventEmitter.js` | 18 — on/off/once, emit, scopes, symbols |
| `core/Printer.js` | 18 — renderer resolution and the default renderers |
| `core/Color.js` | 16 — parsing, luminance, OKLab round trips |
| `core/URLSearchParams.js` | 14 — structured parameters |
| `core/path.js` | 11 — split/join, simplify, relative/absolute |
| `core/EventBinding.js` | 7 — scope and invocation |
| `math/math.js` | 5 — GCD/LCM and the flag layout |
| `schemas/schemas.js` | 2 — type extraction and the official draft 2020-12 suite |
`tests/js/core/path.js` had a `testSimplify` written for a `$N` parent-reference syntax the
implementation dropped in favour of `..`; it had been failing ever since. Rewritten against
the real convention.
### Running them
The browser runner is unchanged — `tests/js/index.php`, Closure's `MultiTestRunner`.
A second runner now lives at `tests/js-node/`, which executes **the same test files** under
node:
```bash
node tests/js-node/run.js # everything
node tests/js-node/run.js core.Map # one namespace
```
It shims `goog`, the jsunit assertions, and `MMO.window`'s run queue (faithfully, frame
semantics included). `jsdom` is picked up *if it happens to resolve* and the four
DOM-bound suites then run too; without it they report as browser-only and the other 242
still run. Nothing was added to the project's dependencies.
---
## 4. Documentation
488 exported functions, methods and accessors had no doc comment. All of them do now,
except two families where one block covers the set better than 27 near-identical stubs:
`Schema.test_*` (one comment stating the shared signature and dispatch rule) and `Entity`'s
six status predicates.
The comments say what is *not* obvious from the signature — the argument order trap in
`Date.compare`, why `MAP_CONNECT` is synchronous, why `loopInputs` cannot rely on
`willValidate`, that `isSimilar` treats arrays as ordered subsequences — rather than
restating the parameter names.
---
## 5. Simplification
Proposals. None of them are applied.
### 5.1 Retire `MMO.core.watch` / `unwatch` / `link` / `linkFn`
Four functions in `functions/core.js` that redefine a property with a getter and a setter,
plus the `MMO.core.UNWATCH_EVENT` sentinel — you remove a watch by *assigning a symbol to
the property*. They predate `MMO.core.Map` and duplicate what it does properly. Nothing in
`src/` outside `core.js` calls them.
Deleting them removes a whole parallel reactivity mechanism, one exported symbol and the
`descriptor.writable` edge case in `watch`.
### 5.2 Collapse the two "is this value like that one" ladders
`isEqual` and `isSimilar` are ~100 lines of near-duplicate type dispatch, and `isSimilar`'s
array branch is a hand-rolled subsequence matcher whose two loop variables share one
counter. `Map.isEqual`, `Map.isSimilar`, `isUnique`, `includes(v, true)` and `test_enum` all
route through them.
One dispatcher parameterised by a leaf comparator (`===` vs `==`) and a containment flag
would be a third of the size, and would make the *ordered subsequence* rule explicit rather
than emergent.
### 5.3 Fold `MMO.core.Template` into `MMO.elements.View`
`Template` has no user of its own. It is instantiated directly only by its own
`RenderTemplate`, and `View` is the only subclass. The split costs a `GlobalRenderers`
registry per class, two `Define`/`IsDefined` pairs, and a `getScope` that has to reach
through `parent` because the base class knows nothing about view nesting.
If the separation is meant to stay, the base class should stop hard-coding `MMO.core.Template`
in its statics (fixed) and the two registries should be documented as deliberately separate.
### 5.4 Give `history.js` an owner, or delete it
`MMO.history` is commented out in `mmo.js`, so `UIAction.execute()` calls `MMO.history.add`
on `undefined`. `UIAction` chains `.then()` on `Action.execute()`, which returns a plain
boolean — only the async subclasses return promises. Three of the API methods it called had
been renamed (fixed). It has no callers.
It is either a feature that was never finished or one that was abandoned. 220 lines either
way.
### 5.5 Split `functions/dom.js`
1341 lines in one namespace, covering selectors, attributes, children, focus, ranges and
rich-text wrapping, styles, unit conversion, coordinates, visibility and dom paths. The
range/selection group (`loopRangeNodes`, `toggleRangeTextWrapping`, `isWrapped`,
`getMouseEventCaretRange`, `selectRange`) is a self-contained rich-text editor helper used
by nothing else — `core/functions/richtext.js` would take ~200 lines out and make both
halves navigable.
### 5.6 Normalise the unit helpers
`decodeUnit(node, value, em, percent)` and `encodeUnit(node, value, unit, percent)` are not
inverses: one takes the reference values as separate arguments, the other infers `%` from
the *type* of `unit` (a number means percent). Both had bugs in the same two branches — a
shape that invites them. One `convertUnit(node, value, from, to, refs)` with an explicit
reference object would be harder to get wrong.
### 5.7 `MMO.core.Map` is 1250 lines doing four jobs
Container, event emitter, watch registry and schema binder. The watch machinery alone
(`watch`, `unwatch`, `connectWatch`, `disconnectWatch`, `changeWatchPath`,
`onChildConnection`, `disconnectScope`, `MAP_WATCHES`) is ~250 lines storing its state as a
**4-element array** `[connected, path, event, bind]` indexed positionally at 23 call sites:
```js
if (this[MMO.core.MAP_WATCHES][i][0]) {
this.disconnectWatch(this[MMO.core.MAP_WATCHES][i][1],
this[MMO.core.MAP_WATCHES][i][2],
this[MMO.core.MAP_WATCHES][i][3]);
}
```
A four-field object (`{connected, path, event, bind}`) costs nothing and makes those sites
readable without counting. Extracting the group into a `MapWatches` helper owning the
array would take the class under 1000 lines without changing a single semantic.
This is the one refactor worth doing on the file the rest of the library depends on — and
now that `core/Map.js` has 53 tests, it can be done with a safety net.
### 5.8 Small things
- `Printer.render(value, opts = [])` and `setRenderer(format, fn, opts = [])` default to an
**array** where the code reads object properties. `{}` is the intent.
- `Map.toObject`, `Entity.clone`, `pathOf` and `Map.From` each thread their own `_hash`
cycle guard through the recursion, four slightly different ways.
- `mmo.js` holds the url helpers, six fetch wrappers, the console loggers, clipboard,
print, download and two browser fixes. The fetch family (`fetch`, `fetchJSON`, `fetchAPI`,
`fetchText`, `fetchXml`, `fetchSvg`, `fetchFile`, `loadJSON`) is ~150 lines with one
concern and belongs in `core/functions/fetch.js`.
- `README.md` says a test file starting with `_` is ignored; `tests/js/index.php` matches
`/\.js$/` with no such exclusion. One of the two is wrong.