Proposal: make derived store APIs type-safe and ergonomic - #3194
Conversation
🦋 Changeset detectedLatest commit: 8f6f99d The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@ryansolid This PR is not specifically intended to implement anything from @brenelz proposal. I think the ergonomics of such recursive mapping might need more investigation. There is probably some overlap in what they solve but they are mostly about different things. The goal of this PR is to improve the ergonomics and type safety for the primitives them selves and doesn't concern it self with later consumption of the store in the app like brenelz issue. I think the runtime guards especially might need your consideration as that goes beyond a simple type change. The AI also claimed it found and fixed SSR bugs along the way that allowed access to the seed value. I have little clue if these fixes are correct. |
ryansolid
left a comment
There was a problem hiding this comment.
Thanks for the clarification. I initially conflated this with #3092 because both use a marker around Store<T>; that was the wrong frame, so I removed that comment and reviewed this proposal independently as a derived-store seed/initialization contract.
There are valuable pieces here, but I don't think the current combined design is safe to continue as one implementation. My recommendation is to split it before further work.
1. Type/API blockers
The source marker does not mean “exactly this Store.” Mapped transformations preserve it, so it contaminates transformed values:
const [source] = createStore<State>(initial);
createStore(omit(source, "mutable")); // no overload matches
const extended = merge(source, { extra: true });
const [, setExtended] = createStore(extended);
setExtended(draft => { draft.extra = false }); // extra is lost; T collapses to StateThe marker is useful for exact source recovery, but omit, merge, built-in mapped types, and intersections need to strip or recompute it rather than carrying a stale source identity.
The unseeded callback type is dishonest. It remains (draft: T) => void | T | ..., while the runtime intentionally makes every first-run draft operation throw. These guaranteed-invalid programs compile:
createProjection<State>(() => {}); // can never initialize
createProjection<State>(draft => ({ count: draft.count })); // first read always throwsA value that cannot be observed as T should not be passed as T. Runtime validation cannot repair that static contract.
Seeded overloads permit callable roots because NoFn<T> is missing from the seeded forms. The draft proxy is not callable, so accepted code fails at runtime. This also contradicts the PR description.
Requiring a complete seed rather than Partial<T> is otherwise directionally correct: if the callback receives draft: T, the seed must establish a real T.
2. Client runtime blockers
The gated draft membrane can be bypassed through exported $TARGET. Internal setter machinery needs that symbol before initialization, but user code can read it too, return the internal target, pass object validation, and expose internals. The proxy also cannot hide typeof, Array.isArray, or identity. “A stable T draft that rejects every observable operation” is not implementable with this shape.
Initialization is tracked outside the scheduler. The closure-local initialized flag opens immediately after write/wrapCommit returns, even when truth is only staged in _pendingValue/store pending backing and still transaction-held or masked. That can enable the public setter/draft before the first value is observable. Core commit state must be the authority; a second state machine will drift.
The optimistic first-landing path conflicts with current next. The PR predates #3146. Its runAuthoritative(write) initialization branch bypasses the now-required flight-owned transaction path (declareFlight / enterFlightTransition), risking premature reveal and breaking fold/until() composition. This cannot be resolved mechanically during rebase.
The gates also run for every already-seeded projection, adding closure/branch overhead and changing reflection behavior where no initialization protection is needed.
3. Server/hydration blockers
There are real baseline SSR bugs here worth extracting: pending server proxies expose seeds through reflection/symbol operations, and rejected projections can become readable as their seed because error handling marks them ready. Those are valid hardening targets.
The proposed membrane is incomplete, though. It omits fundamental proxy traps (getPrototypeOf, setPrototypeOf, isExtensible, preventExtensions). Object.preventExtensions() can succeed while pending and later make Reflect.ownKeys() permanently throw a proxy-invariant TypeError.
Server first-value adoption also diverges from the client: Object.keys/Object.assign leave omitted symbol keys behind and erase class prototypes/methods. Either unseeded values must be restricted to plain records, or adoption must preserve the supported object contract.
Changing client-only hydration from “serve the complete seed” to “hide it and suspend unless seedLoadingValue” is a new public semantic, not an SSR bug fix. It needs an explicit ruling and separate release treatment.
There is also an existing synchronous hydration replay path where processing failures can escape while its promise remains pending. Useful to fix, but separate from this API proposal.
Recommended decomposition
- Source recovery + complete seeded forms (types first). Fix marker contamination, exclude callable roots consistently, and add
merge/omit/mapped-type/intersection probes. Keep completeTseeds; don't restorePartial<T>under a(draft: T)contract. - Redesign the unseeded API without a fake draft. The honest first phase is return-only:
() => T | Promise<T> | AsyncIterable<T>, with no initialvoid. If post-initialization draft mutation is required, expose that as an explicit second phase/updater contract rather than pretending a gated object isT. - Extract the confirmed SSR hardening separately. Complete all fundamental proxy traps/invariants, preserve the supported object shape, and add reflection/symbol/class/error-stream tests.
- Only then design hydration/optimistic integration on current
next. It must use scheduler commit state and the flight-owned transaction model, not parallel initialization flags.
So: the complete-seed and source-recovery goals are worth pursuing, and parts of the SSR investigation found genuine bugs. The seedless runtime/state-machine portion should be replaced rather than patched incrementally. Given the stale base and current semantic conflicts, I would not rebase this branch wholesale; split the proposal into independently reviewable pieces first.
|
I find my self a bit confused about your response but this is how I parse it:
My goal with this draft PR is to do a API design exploration for how to have a overload where you don't need a full seed but that is also type safe given I found the One finding in this exploration is that arrays mostly don't have a problem with providing seed as an empty array is almost always fine and easy to provide. Tuples being the exception. Avoiding arrays by saying they always need a seed opens up additional design space where we can let omitted or nullish seed mean uninitialized object in a way that is known both at compile time and runtime easily. Given these constraints I see a couple of different options that keep the API type safe: Option 1: Optional draft
|
|
This might be controversial but my honest thoughts after having tried to solve the merge/omit issue is that Store making things readonly isn't worth the downstream issues it causes in types. This PR tried to solve it in the simple case with the brand allowing me to extract the original T however that caused issues with merge/omit and still didn't solve other issues like how a store remains readonly when stored as part of another store. I choose to make Store a transparent noop for now and removed the brand as it no longer solves anything this PR is trying to solve. |
43da82a to
74a4138
Compare
|
I’ve split the seeded client-store hydration behavior out into #3223 and removed it from this branch so it can be reviewed independently. |
|
I’ve split the server projection rejection-state fix into #3224. That PR keeps the existing proxy target and reflection/symbol behavior and only ensures rejected pending projections continue to expose their original error. |
4856e06 to
0fb336b
Compare
0fb336b to
8f6f99d
Compare
Merging this PR will degrade performance by 54.89%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
|
One issue I missed in the current implementation is that seedless hydration replay needs a mutation phase that the proposed API currently has no way to represent. The first replay entry is a complete snapshot and can be adopted directly as the store's backing. Later entries are serialized patches, though, so replay needs a draft to apply them to. It effectively needs two phases: // first value
() => T
// later replay entries
(draft: T) => voidThe public seedless overload is return-only, while the projection runner currently uses one How this should be fixed depends on which direction we choose for the seedless API: it could remain return-only with a separate internal replay phase, or we could design an explicit post-initialization mutation phase. I think we should defer implementing this until that API direction is decided rather than add another provisional initialization state machine here. I'm leaving this comment so the current limitation is explicit. |
Status
Important
Both the implementation and this PR description were written with substantial AI assistance. They should be treated as an exploratory API draft that requires careful human review, not as ready-to-merge work.
The independently extracted fixes in #3223 and #3224 have now merged. This branch is rebased directly onto the latest
next; those fixes are intentionally not described below as part of this proposal.Summary
This explores a stricter and more ergonomic contract for the store primitives around two related problems:
Store<T> = Readonly<T>means passing an existing store into another store primitive can infer the newTas the already-readonly store type. This makes setter drafts unexpectedly readonly and makes mapped helpers, intersections, and repeated store wrapping difficult to model without increasingly complex type machinery.seed: Partial<T>while exposing both the callback draft and resulting store as completeT. A partial seed does not satisfy that contract unless user code happens to initialize every required property before it can be observed.Proposed API
1. Keep
Store<T>as a documentation alias forTThis removes the synthetic top-level
Readonly<T>transformation. Reusing stores therefore preserves normal inference without a private source brand:It also lets
omit,merge, built-in mapped types, intersections, and third-party helpers operate on the structural value without carrying stale store identity metadata.This is intentionally less strict:
Store<T>no longer adds readonly properties that were not present inT. User-declaredreadonlyfields remain readonly because the originalTis preserved.2. A provided seed must be a complete
TThe seeded form receives a real
Tdraft and may mutate it, return a replacement, or returnvoid:The seed is
T, notPartial<T>, because it is immediately exposed to the callback asT. The same rule applies tocreateStore(fn, seed),createProjection(fn, seed), andcreateOptimisticStore(fn, seed).Arrays and tuples use this seeded form. An empty array is normally a natural complete seed for an array projection:
seedLoadingValue: trueis also limited to the seeded form because only a complete seed can honestly be exposed as commit zero.The old mutation-based empty-object initialization remains available as an explicit escape hatch:
The cast is required because TypeScript and the runtime cannot prove that every required field is established before observation. Once the caller casts, they take responsibility for that invariant.
3. An omitted or nullish seed selects a return-only initializer
For non-array object roots, a seed can be omitted:
The seedless callback receives no draft and must return, resolve, or yield a complete
Tevery time:This makes the first phase honest without passing a value typed as
Tbefore anyTexists. Runtime validation rejectsvoid,null, primitives, and arrays if the type system is bypassed. Array projections require a real seed.A nullish placeholder is only needed to reach the options argument:
The writable seedless forms keep their public setter unavailable until the first complete value has initialized the store. Hydration may carry a private replay backing, but that implementation detail does not turn the public seedless form into a seeded callback or make its setter available early.
This PR currently implements the return-only option suggested in review. The broader question of whether seedless projections should instead expose an explicit second mutation phase, or use another shape entirely, is discussed in this comment.
4. Apply the same overload model to each store primitive
createStore(value, options?)andcreateOptimisticStore(value, options?)create plain stores.createStore(fn, seed?),createProjection(fn, seed?), andcreateOptimisticStore(fn, seed?)share the complete-seed versus return-only-seedless distinction.draft: T; seedless callbacks receive no arguments.Runtime implementation
The same seeded/seedless distinction is carried through the client, server, hydration, and optimistic implementations:
voidFeedback requested
Readonly<T>transformation fromStore<T>an acceptable tradeoff for straightforward inference and composition?Tseed or return-only seedless initializer” the right overload boundary?How did you test this change?
From
packages/signals:tests/store/store.type-tests.tspnpm typesFrom
packages/solid:pnpm test-typespnpm types