address architecture review findings and add review document

Fixes from architecture review (4 critical, 10 warnings):

Critical:
- Fix selectNode/setNode docs to accurately describe prop-key
  navigation behavior including array support and prop-key writes
- Document RenderContext/Density exported types in host-config
- Resolve ADR dual status ambiguity with clarifying note in README
  (frontmatter status = editorial, body Status = decision)
- Effect types already addressed in prior commit

Warnings addressed:
- Add Fragment re-export note to jsx-runtime section in
  build-distribution
- Document childCtx/transformCtx helper functions in transforms.md
- Document render() accepting non-root UNode in host-config
- Add Value.Hash re-entrancy constraint to reconciler.md
- Add true-passthrough constraint and h('root') special case
  to element-factory constraints
- Add _idCounter bundling caveat note

Review document added at docs/reviews/architecture-review-2026-05-18.md
with full findings, source verification table, and recommendations.
This commit is contained in:
2026-05-18 15:36:38 +00:00
parent da82b52b27
commit 23659233ca
8 changed files with 322 additions and 7 deletions

View File

@@ -116,7 +116,7 @@ last_updated: YYYY-MM-DD
| `stable` | API contracts are locked. Changes require a review cycle and may warrant an ADR if they affect documented decisions. | → `deprecated` when superseded. → `draft` if a fundamental redesign is needed (rare). |
| `deprecated` | Superseded by another document. Kept for reference. Links should point to the replacement. | Removed when no longer referenced. |
ADR documents use a separate `Status` field in their body: `Proposed`, `Accepted`, `Deprecated`, or `Superseded`. ADRs never revert from `Accepted`.
ADR documents use a separate `Status` field in their body: `Proposed`, `Accepted`, `Deprecated`, or `Superseded`. ADRs never revert from `Accepted`. Note that ADR frontmatter (`status: draft`) refers to the **document's editorial status** (is the writing complete?), while the body `Status` refers to the **decision's status** (is this decision finalized?). A `Proposed` decision can have a `draft` document; an `Accepted` decision has a `stable` document.
## References

View File

@@ -81,6 +81,8 @@ The `./jsx-runtime` export enables `jsxImportSource` configuration. When a consu
TypeScript and other JSX transforms resolve `jsx`, `jsxs`, and `jsxDEV` from `@alkdev/ujsx/jsx-runtime`. All three are aliases for `h()` — UJSX does not distinguish between static children, dynamic children, or dev mode at the factory level.
The `jsx-runtime` export also re-exports `Fragment` from `h.ts`. This means JSX consumers can import `{ Fragment }` from `@alkdev/ujsx/jsx-runtime` as well as from `@alkdev/ujsx/h` or the barrel export.
## Dependencies
| Package | Version | Role | Hard/Peer? |

View File

@@ -141,10 +141,11 @@ This is documented in the schema architecture ([schema.md](schema.md) — Known
## Constraints
- **`h()` is pure** — no side effects beyond the `_idCounter` increment in `createRoot()`. It does not call hosts, subscribe to signals, or mutate external state.
- **Children are always flat** — `flat(Infinity)` + filter means consumers never receive nested arrays or null/false children from factory output. Hosts and transforms can assume `element.children` is a flat `UNode[]` with no null slots.
- **`h("root", ...)` is a special case** — the string `"root"` is effectively a reserved type string. When `type === "root"`, `h()` produces a `URoot` (discriminated union with `type: "root"`), not a `UElement`. This is not a host tag — no host will ever receive `"root"` as a `createInstance` tag.
- **Children are always flat** — `flat(Infinity)` + filter means consumers never receive nested arrays or null/false children from factory output. Hosts and transforms can assume `element.children` is a flat `UNode[]` with no null slots. Note that `true` values are NOT filtered — `{condition}` where `condition` is `true` produces a `true` `UPrimitive` child. Hosts that want `true` to render as nothing should filter it in their `createTextInstance`.
- **Props are not deep-cloned** — `h()` spreads props shallowly. Nested objects are shared references. Consumers must not mutate element.props and expect isolation.
- **`Fragment` produces arrays, not elements** — hosts and transforms must handle `UNode[]` return values from component renders. A Fragment does not appear in the tree.
- **`_idCounter` is module-scoped** — each module instance has its own counter. If multiple copies of UJSX are loaded (e.g., different package versions), roots from different copies may collide on `id` values.
- **`_idCounter` is module-scoped** — each module instance has its own counter. If multiple copies of UJSX are loaded (e.g., different package versions), roots from different copies may collide on `id` values. Bundle deduplication behavior determines whether copies share the counter.
- **JSX aliases are identical** — `jsx`, `jsxs`, and `jsxDEV` are the same function. UJSX does not differentiate between them. Dev-mode only features (e.g., source location) are not currently supported.
## References

View File

@@ -190,8 +190,10 @@ The reconciler solves this by maintaining a fiber tree alongside the instance tr
- **`HostConfig` is the sole host integration point** — all platform-specific logic lives behind this interface. UJSX never calls DOM APIs, Three.js APIs, or any other platform API directly.
- **`TTag` constrains element types** — hosts declare the tags they support. Attempting to render an unsupported tag is a type error at compile time. At runtime, the host receives any string as `tag` and must handle unknowns.
- **`render()` is not idempotent** — calling `render()` twice on the same root creates two independent instance trees. It does not update the first tree.
- **`render()` accepts any `UNode`** — if the node is a `URoot`, its children are mounted directly. If the node is any other `UNode` (element or primitive), it is wrapped in an array and mounted as a single top-level element without a root container.
- **Function components are synchronous and transparent** — they receive props and children, return a `UNode`, and produce no host instance. The reconciler research discusses how to handle components that return different tree shapes across renders.
- **`Context` is always present** — `createRoot()` guarantees a `Context` exists on the `Root`. If none is provided, a default is created with `density: "full"`, `target: "markdown"`, and empty `metadata`.
- **`RenderContext` extends `ContextValue`** — the `Direction` type (`"ujsx→mdast" | "mdast→ujsx" | "ujsx→jpath" | "jpath→ujsx" | "ujsx→hast" | "hast→ujsx"`) and `RenderContext` interface (which adds `direction` to `ContextValue`) are exported from the `context` sub-path. `RenderContext` is primarily used by the transform system to specify conversion direction. `Density` (`"full" | "compact" | "minimal"`) controls rendering granularity for hosts that support adaptive output.
- **`container` is opaque** — UJSX passes it to `createRootContext` and stores it on `Root`, but never inspects it. The host defines what it means.
- **Mount is depth-first, post-order** — children are fully constructed before being appended to their parent. Hosts can rely on this ordering invariant.

View File

@@ -64,17 +64,32 @@ Each segment is processed sequentially against the current node:
| Segment | Resolution | Example |
|---------|-----------|---------|
| Numeric (e.g., `"0"`, `"3"`) | `children[index]` on the current `UElement` | Path `["0", "2"]` → root.children[0].children[2] |
| Non-numeric (e.g., `"title"`) | `props[segment]` on the current `UElement`, if the prop value is an object | Path `["props", "title"]` → root.props.title (if title is an object node) |
| Non-numeric (e.g., `"title"`, `"items"`) | `props[segment]` on the current `UElement` if the prop value is a non-null object (including arrays), navigation continues into that value; otherwise returns `undefined` | Path `["items"]` → root.props.items (if items is an object or array) |
A segment that parses as a valid non-negative integer is treated as a children index. Otherwise, it is treated as a prop key. This is a simplified version of RFC 6901 JSON Pointer — no special escaping, no `~` encoding, no `/` separators. Simplicity over generality.
A segment that parses as a valid non-negative integer is treated as a children index. Otherwise, it is treated as a prop key. If the prop value exists and is a non-null object (which includes arrays, since `typeof [] === "object"`), `selectNode` navigates into it. If the prop value is a primitive (string, number, boolean, null) or absent, `selectNode` returns `undefined`.
This is a simplified version of RFC 6901 JSON Pointer — no special escaping, no `~` encoding, no `/` separators. Simplicity over generality.
### Early termination
If the current node is not a `UElement` (i.e., it's a `UPrimitive` — a string, number, boolean, or null), `selectNode` returns `undefined` because primitives have no children or props. This prevents runtime errors from navigating into leaf values.
### Non-element props
### Non-element and array props
When a string segment resolves to a prop value that is not an object (e.g., `props.title` is a string), `selectNode` returns `undefined`. Only object-typed prop values can be navigation targets. This is because `UPrimitive` values (strings, numbers) are terminal — they have no children to navigate into. A prop that holds a `UNode` subtree is an object and can be navigated; a prop that holds a string is a leaf.
When a string segment resolves to a prop value that is a primitive (string, number, boolean, null), `selectNode` returns `undefined`. Only non-null object prop values — including arrays, since `typeof [] === "object"` — can be navigation targets. This means `props.items` where `items` is an array can be navigated into, but `props.title` where `title` is a string cannot.
`setNode` mirrors this behavior for writes: a non-numeric string segment sets `props[segment] = value`, performing a shallow merge of that key into the element's props. This allows targeted prop updates via path-based navigation.
### setNode prop-key writes
Non-numeric path segments in `setNode` set values into the `props` object:
```typescript
setNode(root, ["title"], someNode)
// Produces: { ...rootEl, props: { ...rootEl.props, title: someNode } }
```
This shallow-merges a key into `props`. The `value` must be a valid `PropValue` (or `UNode`) for the result to remain type-safe.
## setNode

View File

@@ -244,6 +244,7 @@ Host implementations that currently only support mount-only rendering will start
- **`key` is a reconciler concern, not a prop** — `key` is extracted by `h()` and never passed to components or hosts. See [ADR-004](decisions/004-key-as-first-class-field.md).
- **Commit order is parent → child, top-down** — this ensures parent state is consistent when child updates fire.
- **TypeBox optimizations are incremental** — the reconciler is correct without them. Each optimization layers on top.
- **`Value.Hash` is not re-entrant** — it uses a global mutable accumulator. It must not be called from within a `computed` or `effect` that is itself triggered by a hash comparison. Hashes must be computed outside reactive computations, during the commit phase.
- **Phase 5 (flowgraph host configs) belongs in `@alkdev/flowgraph`** — ujsx stays generic. Flowgraph is a consumer, not a modification.
## Open Questions

View File

@@ -93,6 +93,18 @@ The "first match wins" semantics mean that priority and registration order resol
Maps `transform()` over an array, passing each node's index as `ctx.index`. This is a convenience for transforming child lists — it preserves the ancestor stack from `ctx` without requiring callers to manage index tracking.
## Helper Functions
The transform module exports two context factory functions used alongside `TransformRegistry`:
### `ctx<A>(direction, ancestors?, index?, metadata?)`
Creates a `TransformContext` from its arguments. The `direction` parameter is required; the rest default to `[]`, `0`, and `{}` respectively. Exported as `transformCtx` from the barrel (`@alkdev/ujsx/transform`) to avoid name collision with React's `ctx` naming.
### `childCtx<A>(parent, ctx, index)`
Creates a new `TransformContext` with `parent` pushed onto the ancestors stack and `index` set. This is the standard way to descend into a child node during transformation — it preserves the direction and metadata from the parent context while updating traversal state.
## Direction Definitions
The six directions pair into three bi-directional channels: