Skip to content

Sever upward eval-time edges: late-bind XH out of low-level modules #4643

Description

@amcclain

Part of #4640 - the actual fix for the boot-crash class. Design work, not mechanical. Gated on #4641's harness; gates #4644.

Goal

Core primitives (HoistBaseDecorators, HoistBase, promise package) must never statically evaluate XH's import chain, so that no evaluation order can hit a decorator or extends before its binding initializes.

Known chokepoints (both verified crash paths route through these)

  • promise/Promise.ts -> XH (drags the world into HoistBaseDecorators' eval chain via its wait import)
  • core/runner/Runner.ts -> XH (drags the world into HoistBase's eval chain)

The #4641 audit may surface additional upward edges with eval-time consequences.

Every XH. reference in both chokepoints is inside a method body - none at module scope:

  • promise/Promise.ts - L236, L245, L266, L278, L284, all within catchDefault() and track()
  • core/runner/Runner.ts - L121-L189, all within fetch(), fetchJson(), getJson(), postJson(), putJson(), patchJson(), deleteJson(), withSpan() and the metrics path

That is what makes late binding available at all: the static import {XH} is a load-time edge serving purely call-time uses. Nothing in these modules' bodies needs XH to exist at evaluation time.

Note this is late binding, not lazy evaluation - the value is still created eagerly at bootstrap exactly as today; only the reference to it resolves at call time. Nothing is computed on demand or memoized. The property that fixes the crash is that the reference stops being an import edge.

Approach: explicit accessor on a leaf module

// utils/impl/XHRef.ts - a leaf. No runtime imports from hoist beyond utils.
import type {XHApi} from '@xh/hoist/core';
import {throwIf} from '@xh/hoist/utils/js/LangUtils';

let _xh: XHApi = null;

/** Called once from the bottom of core/XH.ts. */
export function setXH(xh: XHApi) { _xh = xh; }

export function getXH(): XHApi {
    throwIf(!_xh, 'XH accessed before app bootstrap.');
    return _xh;
}

Call sites become getXH().track(opts) - 14 of them, 5 in Promise.ts and 9 in Runner.ts. Type-only imports from @xh/hoist/core (TrackOptions, ExceptionHandlerOptions, TaskObserver) stay and are erased by Babel once #4642 lands, leaving these modules with zero runtime import of core/XH.ts.

App code's import {XH} from '@xh/hoist/core' is untouched. This is not a public API change.

Precedent: core/XH.ts already ends with window['XH'] = XH - an untyped, single-global version of the same move, currently used only for devtools.

Rejected: a Proxy preserving the XH. idiom

Exporting the slot as a Proxy also named XH would keep every call site verbatim, changing only the import line. Tempting - XH was made short and punchy on purpose, and easy for an IDE to import. Rejected anyway:

  • A get-only proxy handles property access and destructuring correctly - and this codebase destructures XH freely ({darkTheme, isDesktop} = XH, {router} = XH, {isMobileApp} = XH, across a dozen-plus files) - but its immediate neighbours fail, and fail silently: {...XH} -> {}, Object.keys(XH) -> [], 'router' in XH -> false. Each needs ownKeys / has / getOwnPropertyDescriptor traps.
  • A silent wrong answer is a worse failure mode than the boot crash we are fixing, which at least announces itself.
  • Additionally: MobX's @action on XHApi through a bound proxy is untested; the debugger shows a Proxy rather than the object; and the get trap needs Reflect.get(_xh, prop, _xh) plus method binding to keep 27 getters and their receivers correct.

At 14 call sites in two files, having two things named XH that behave identically until they don't is the worse trade. The explicit call is also a visible marker that you are inside the layering-constrained zone - in exactly the files where a reader most needs to know.

What we pay instead: IDE auto-import will offer import {XH} from '@xh/hoist/core' in these files - the one that reintroduces the cycle. Not preventable, only correctable, via the rule below. (Note core/index.ts's existing comment: "Explicitly exporting XH helps IntelliJ suggest the correct import from this core package." Someone hit this before.)

The rubric: when getXH(), when XH

Positional, not behavioural. A behavioural rule ("use getXH() when you would otherwise create an eval-time cycle") is unusable - it asks the developer to hold a 563-cycle graph in their head. A path check is decidable in one second, by a person or a coding agent:

Is this file in the foundation set? If yes, getXH(). If no, XH as normal.

Enforced both ways in eslint.config.js:

const FOUNDATION = [
    'utils/**', 'promise/**',
    'core/HoistBase.ts', 'core/HoistBaseDecorators.ts', 'core/runner/**'
    // + whatever the #4641 audit adds
];

const MSG = 'Foundation modules must not statically import XH - it creates an eval-time ' +
            'cycle (#4643). Use getXH() from @xh/hoist/utils/impl/XHRef.';

{
    files: FOUNDATION,
    rules: {'no-restricted-imports': ['error', {paths: [
        {name: '@xh/hoist/core',    importNames: ['XH'], message: MSG},
        {name: '@xh/hoist/core/XH', importNames: ['XH'], message: MSG}
    ]}]}
},
{
    // symmetric - keeps the wart from spreading
    files: ['**/*.ts', '**/*.tsx'],
    ignores: FOUNDATION,
    rules: {'no-restricted-imports': ['error', {paths: [
        {name: '@xh/hoist/utils/impl/XHRef',
         message: 'getXH() is only for foundation modules - use XH from @xh/hoist/core.'}
    ]}]}
}
  • importNames: ['XH'] bans the symbol, not the barrel - import type {TrackOptions} from '@xh/hoist/core' keeps working, which matters because that is most of what these files import.
  • Two path entries, because De-barrel internal imports + lint/CI ratchet #4644 rewrites the barrel form into the deep form.
  • Globs are positional, so a file added to promise/ next year is covered on creation. Nobody has to remember the rule.
  • The lint message is the documentation - for humans and for coding agents, which read eslint.config.js and CLAUDE.md long before a closed issue.

Two gaps to be explicit about:

Establish the intended layering explicitly (utils -> promise -> core primitives -> models/services -> XH -> platform packages) so the change has a documented rationale and future edges have something to violate.

Risk

The failure mode changes shape rather than vanishing: instead of a load-time ReferenceError, an over-eager caller gets a clear throw at call time if it reaches getXH() before bootstrap has run. That is what the throwIf is for, and why this phase wants real Toolbox boot testing rather than only a green harness.

Done when

The #4641 guard harness passes under sideEffects: 'flag' for the core skeleton exports (managed, persist, HoistBase, HoistModel, HoistService, and friends).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions