Skip to content

Commit 38c32fa

Browse files
committed
Revert #6: keep global hierarchy module-local
Hoisting _globalHierarchy onto globalThis[Symbol.for(…)] was over-defensive. The playground bug we used as precedent was fixed by unifying URLs, not by papering over duplicate loads with shared state. Normal ESM semantics — one module instance per realm per URL — is the correct behavior, and every other module-level state in squint (including core.js's _metaSym) lives that way. cljs.core's *global-hierarchy* is module-scoped for the same reason. A user who ends up with two instances of multi.js under different URLs almost certainly has two versions of squint in their graph; sharing state across versions via globalThis would hide real version-mismatch bugs instead of surfacing them. Move #6 from Done to Wontfix in todo.md with the reasoning.
1 parent 71309b4 commit 38c32fa

2 files changed

Lines changed: 16 additions & 16 deletions

File tree

src/squint/multi.js

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,10 @@ export function make_hierarchy() {
1919
};
2020
}
2121

22-
// Store the global hierarchy on globalThis under a Symbol.for key so all
23-
// instances of this module (e.g. if the bundle ends up loaded twice
24-
// under different URLs — see the playground dual-module episode) share
25-
// one hierarchy. Without this, a `derive` through one instance would be
26-
// invisible to `isa?` queries issued through the other.
27-
const GH_KEY = Symbol.for('squint.multi.hierarchy');
22+
let _globalHierarchy = null;
2823
function gh() {
29-
return globalThis[GH_KEY] ?? (globalThis[GH_KEY] = make_hierarchy());
24+
return _globalHierarchy ?? (_globalHierarchy = make_hierarchy());
3025
}
31-
function setGlobalHierarchy(h) { globalThis[GH_KEY] = h; }
3226

3327
function _isa(h, child, parent) {
3428
if (_EQ_(child, parent)) return true;
@@ -89,9 +83,8 @@ export function derive(a, b, c) {
8983
// invalidate, so in-place mutation would leave stale cached
9084
// resolutions in place (e.g. a subsequent derive can introduce
9185
// ambiguity that the cache would otherwise hide).
92-
const next = cloneHierarchy(gh());
93-
_deriveInto(next, a, b);
94-
setGlobalHierarchy(next);
86+
_globalHierarchy = cloneHierarchy(gh());
87+
_deriveInto(_globalHierarchy, a, b);
9588
return null;
9689
}
9790
const next = cloneHierarchy(a);
@@ -122,7 +115,7 @@ export function underive(a, b, c) {
122115
}
123116
}
124117
if (c === undefined) {
125-
setGlobalHierarchy(rebuildFromPairs(pairs));
118+
_globalHierarchy = rebuildFromPairs(pairs);
126119
return null;
127120
}
128121
return rebuildFromPairs(pairs);

todo.md

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,17 @@ Captured from PR feedback on the `multimethods` branch.
1414
change as potentially breaking and explains the migration.
1515
- **#5** `defmulti` now auto-wraps a plain `(make-hierarchy)` passed as
1616
`:hierarchy` so `.deref()` dispatch doesn't crash. Regression test.
17-
- **#6** `_globalHierarchy` moved onto
18-
`globalThis[Symbol.for('squint.multi.hierarchy')]` so dual module
19-
loads (e.g. npm + CDN, symlink quirks) share one hierarchy. No
20-
user-facing test — observable only by reaching into internals.
17+
## Wontfix
18+
19+
- **#6** `_globalHierarchy` dual-module trap. The playground fix for
20+
the `_metaSym` analogue unified URLs rather than hoisting state, and
21+
normal ESM semantics (one instance per realm per URL) is the
22+
correct behavior — `cljs.core`'s `*global-hierarchy*` is
23+
module-scoped for the same reason. A user who ends up with two
24+
instances of `multi.js` under different URLs has a bundler/config
25+
problem (likely two versions of squint in the graph); sharing state
26+
across versions via `globalThis` would hide real version-mismatch
27+
bugs instead of surfacing them. Module-local state stays.
2128

2229
## Defer (file as follow-up issue)
2330

0 commit comments

Comments
 (0)