[lexical] Extend the $config() protocol with accessor-based nominal typing#8645
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…yping ## Description This is the additive, backwards-compatible half of the $config() work, split out so its backwards compatibility can be audited independently of the node-class refactor that follows. No existing node class is changed here: every change extends the $config() protocol, its supporting types and runtime, the documentation, and the tests. Existing classes keep their hand-written statics and are unaffected. Protocol changes (additive): - Abstract base classes have no concrete node `type`, so they can now declare configuration shared with their subclasses (e.g. a `$transform` or required NodeState) under a well-known `Symbol.for(<ClassName>)` key, which `getStaticNodeConfig` resolves via `Object.getOwnPropertySymbols`. `BaseStaticNodeConfig` becomes a single mapped type over `string | symbol`, and `StaticNodeConfigValue`'s `Type` parameter is widened to `string | symbol` (a symbol-keyed config never populates the string `type` field). - TypeScript is structural, so a node subclass that adds no type-distinguishable members over its base (e.g. `TabNode` over `TextNode`, which only overrides methods with identical signatures) is indistinguishable from it, collapsing the negative branch of guards like `$isTabNode()` to `never`. A node that declares its `extends` now records its own `type` in the `$config` return type under an internal, type-only `() => Type` accessor that accumulates down the `extends` chain by call-signature intersection: the base is no longer assignable to the subclass (so guards narrow correctly) while the subclass stays assignable to the base, and the distinction holds at every level of the hierarchy. The accessor key lives behind an `@internal` `StaticNodeTypeAccessor` interface so inferred `$config()` return types stay nameable in generated `.d.ts`. `GetStaticNodeConfig` reads a node's own config out of the now-accumulating record by indexing its resolved own `type`. - The default `LexicalNode.importJSON` parameter is widened to `SerializedLexicalNode & Record<string, unknown>` so a node relying on the synthesized `importJSON` accepts its own richer serialized shape — extra subclass fields and all — without a cast. Tests: - Coverage for the abstract-class Symbol `$config` mechanism, `importJSON()` and `clone()` with no boilerplate, and a narrowing regression test asserting that `$isTabNode()`'s negative branch on a `TextNode` stays `TextNode` rather than collapsing to `never`. Docs: - `concepts/nodes.mdx`: declaring shared `$config` on an abstract base class under a `Symbol.for(...)` key, and the automatic nominal typing that distinguishes structurally-identical nodes once they declare `extends`. ## Test plan `pnpm run tsc`, `pnpm run flow`, ESLint, and Prettier all pass with the node classes unchanged. The full unit suite is green: 3729 passed / 1 skipped across `--project unit` and `--project scripts-unit`.
8367948 to
ed8e355
Compare
potatowagon
left a comment
There was a problem hiding this comment.
Review from Navi (Sherry's AI assistant)
This is a well-designed extension to the $config() protocol that solves two real problems:
1. Symbol-keyed config for abstract base classes — Abstract nodes (e.g. ElementNode, DecoratorNode) can now declare shared configuration ($transform, state) under Symbol.for('<ClassName>') without needing a concrete node type. The implementation in LexicalUtils.ts correctly falls back to symbol keys via Object.getOwnPropertySymbols() only when no string-keyed config is found.
2. Accessor-based nominal typing — Elegant solution to TypeScript's structural typing limitation. By accumulating () => Type function signatures in an intersection under STATIC_NODE_TYPE, subclasses that are structurally identical to their base (like TabNode vs TextNode) stay nominally distinct. The key insight that ReturnType on an overload set resolves to the last signature (most-derived type) is correct and well-documented in the code comments.
What I verified:
- ✅ The
config()method overloads are correctly separated: symbol key returnsBaseStaticNodeConfig(no type info), string key returns the fullStaticNodeConfigRecordwith accessor - ✅
StaticNodeConfigRecordconditional type correctly chains: whenextendsis present, it intersects the parent's config record + own key +StaticNodeTypeAccessor; withoutextends, it falls back to base behavior - ✅
GetStaticNodeTypeprefers theSTATIC_NODE_TYPEaccessor (resolves viaReturnType) before falling back to the oldStaticNodeConfiginference path — backward compatible - ✅
GetStaticNodeConfigsuses[X] extends [never]to avoid distributive conditional collapse — correct TypeScript pattern - ✅ The
STATIC_NODE_TYPEsymbol isdeclare-only (no runtime cost, never set byconfig()) - ✅
importJSONwidening to& Record<string, unknown>is safe — makes the parameter more permissive for subclasses - ✅ Test mock for
LexicalNode.getTypenow correctly delegates to the real method for subclasses instead of collapsing everything to'node' - ✅ Both new tests exercise the exact scenarios: abstract base w/ symbol key + transform propagation, and structurally-identical narrowing with
expectTypeOf
CI Status: Core tests (unit, browser, integrity, integration) all pass. Most e2e suites pass. A few mac/collab e2e jobs are still pending but no failures detected.
Minor observations (not blocking):
- The
StaticNodeTypeAccessorinterface is exported from the public API surface (index.ts) — intentional for .d.ts namability but worth noting it's@internalin its JSDoc - The docs addition in
nodes.mdxis clear and provides good concrete examples
Verdict: This is safe to land. The type-level changes are well-contained within the $config() protocol, backward compatible (old nodes without extends still work via the conditional fallback), and the runtime change is minimal (symbol key fallback in getStaticNodeConfig). The nominal typing pattern is sound TypeScript and solves a real narrowing problem.
— via Navi on behalf of potatowagon
afb1c11 to
6b1d902
Compare
…classes ## Description A node's `$config()` may be keyed by a `Symbol` so an abstract base class — one with no concrete node `type` — can share configuration (a `$transform`, required NodeState) with its subclasses. Interleaving such a base between concrete classes (`ElementNode -> Paragraph -> AbstractBase -> Concrete`) exposed two type-level bugs, both stemming from the `Symbol` overload of `config()` returning the loose `BaseStaticNodeConfig`: - The `Symbol`-keyed `$config()` override did not type-check (TS2416) when its superclass was a concrete node whose `$config()` return already carried a `STATIC_NODE_TYPE` accessor, because `BaseStaticNodeConfig` lacks that accessor. - `NodeStateJSON` (and `exportJSON`'s inferred type) truncated at the `Symbol`-keyed base: `GetStaticNodeConfigs` stops where `GetStaticNodeConfig` is `never`, and a `Symbol`-keyed base resolved to `never` because it has no string `type` to index the record by — so the base's own required state and all ancestor state above it were dropped from the type, even though the runtime registered and serialized all of it. The fix carries each node's own config under a new `declare`-only `STATIC_NODE_CONFIG` accessor — a `() => Config` that accumulates down the `extends` chain by call-signature intersection, mirroring `STATIC_NODE_TYPE`: - The `Symbol` overload now returns `AbstractStaticNodeConfigRecord`, which inherits its superclass record (and thus that record's `STATIC_NODE_TYPE` accessor, so the override is valid) and exposes its own config under `STATIC_NODE_CONFIG`. - `StaticNodeConfigRecord` (concrete nodes) also exposes its own config there. - `GetStaticNodeConfig` reads the own config from `STATIC_NODE_CONFIG` — which resolves `Symbol`-keyed bases without indexing by `type` — and falls back to the previous type-indexing for a record that sets no accessor (a node with no `extends`, or a legacy node), so its behavior is unchanged for those. The accessor is type-only and never set at runtime, so there is no runtime change; the runtime state/transform collectors already walked the full prototype chain and were always correct. ## Test plan Adds runtime + type tests for an interleaved `ElementNode -> concrete -> Symbol-keyed abstract -> concrete` hierarchy, asserting that every level's flat state is both serialized at runtime and present in `NodeStateJSON` (and that the abstract base's `$transform` runs for the concrete leaf). `pnpm run tsc`, `pnpm run flow`, ESLint, and Prettier all pass. The full unit suite is green: 3730 passed / 1 skipped across `--project unit` and `--project scripts-unit`. https://claude.ai/code/session_01VwdCxD5sn4Ty8ahA9aZnxP
6b1d902 to
1799121
Compare
Description
This is the additive, minimally invasive half of the $config() work, split out so its backwards compatibility can be audited independently of the node-class refactor that follows. No existing node class is changed here: every change extends the $config() protocol, its supporting types and runtime, the documentation, and the tests. Existing classes keep their hand-written statics and are unaffected.
Protocol changes (additive):
type, so they can now declare configuration shared with their subclasses (e.g. a$transformor required NodeState) under a well-knownSymbol.for(<ClassName>)key, whichgetStaticNodeConfigresolves viaObject.getOwnPropertySymbols.BaseStaticNodeConfigbecomes a single mapped type overstring | symbol, andStaticNodeConfigValue'sTypeparameter is widened tostring | symbol(a symbol-keyed config never populates the stringtypefield).TabNodeoverTextNode, which only overrides methods with identical signatures) is indistinguishable from it, collapsing the negative branch of guards like$isTabNode()tonever. A node that declares itsextendsnow records its owntypein the$configreturn type under an internal, type-only() => Typeaccessor that accumulates down theextendschain by call-signature intersection: the base is no longer assignable to the subclass (so guards narrow correctly) while the subclass stays assignable to the base, and the distinction holds at every level of the hierarchy. The accessor key lives behind an@internalStaticNodeTypeAccessorinterface so inferred$config()return types stay nameable in generated.d.ts.GetStaticNodeConfigreads a node's own config out of the now-accumulating record by indexing its resolved owntype.LexicalNode.importJSONparameter is widened toSerializedLexicalNode & Record<string, unknown>so a node relying on the synthesizedimportJSONaccepts its own richer serialized shape — extra subclass fields and all — without a cast.Tests:
$configmechanism,importJSON()andclone()with no boilerplate, and a narrowing regression test asserting that$isTabNode()'s negative branch on aTextNodestaysTextNoderather than collapsing tonever.Docs:
concepts/nodes.mdx: declaring shared$configon an abstract base class under aSymbol.for(...)key, and the automatic nominal typing that distinguishes structurally-identical nodes once they declareextends.A full implementation that includes the refactoring of classes is in #8640
Test plan
New unit tests