fix(storage): types#818
Conversation
docs(storage): resource usage
🦋 Changeset detectedLatest commit: a4ee095 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
thetarnav
left a comment
thetarnav
left a comment
There was a problem hiding this comment.
You really like using Object.assign for some reason.
I find it much harder to read and understand.
What's the idea behind it?
|
The idea is mostly about TS, because it lets you keep the original type information, which is impossible if you generate the tuple yourself. |
|
|
|
The problem with typecasting is that the generics can clash. It is very difficult to get around that, hence why I prefer Object.assign. |
|
@atk what would we like to do this type PR? Perhaps you can close or move it to the new primitives release? |
|
there is also a perf. benefit in v8 to using |
|
I'd like to merge this nonetheless; the new version uses a similar pattern. |
Deploying solid-primitives with
|
| Latest commit: |
a4ee095
|
| Status: | 🚫 Build failed. |
|
@atk there's no official approval but either feel free to merge or give me the approval so I feel comfortable to merge :) thanks! |
📝 WalkthroughWalkthroughThis PR simplifies the ChangesmakePersisted Type and Return Shape
Estimated code review effort: 2 (Simple) | ~12 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/storage/src/persisted.ts`:
- Around line 112-115: The no-storage branch in persisted storage is mutating
the caller-provided signal tuple in place via Object.assign(signal, { 2: null
}), which makes it inconsistent with the storage-available path in persisted.ts.
Update the early return in the persisted logic so it creates and returns a new
tuple/array, matching the cloning approach used later in the storage-enabled
branch, and avoid writing to the original signal object.
- Around line 95-111: The makePersisted overloads are not inferring T from the
signal value, so PersistenceOptions callbacks can fall back to unknown. Update
the makePersisted signature in persisted.ts to derive T from S using the
signal/store element type instead of the current generic constraint, and keep
the overloads aligned so options.serialize, deserialize, and sync are checked
against the actual signal type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 008ce502-5e5a-4bf2-9450-179ca945d9b8
📒 Files selected for processing (4)
.changeset/short-memes-count.mdpackages/storage/README.mdpackages/storage/src/persisted.tspackages/storage/test/persisted.test.ts
| export function makePersisted<T, S extends Signal<T> | [Store<T>, SetStoreFunction<T>]>( | ||
| signal: S, | ||
| options: PersistenceOptions<SignalType<S>, O>, | ||
| options?: PersistenceOptions<T, undefined>, | ||
| ): PersistedState<S>; | ||
| export function makePersisted< | ||
| S extends SignalInput, | ||
| T, | ||
| S extends Signal<T> | [Store<T>, SetStoreFunction<T>], | ||
| O extends Record<string, any>, | ||
| >(signal: S, options: PersistenceOptions<T, O>): PersistedState<S>; | ||
| export function makePersisted< | ||
| T, | ||
| S extends Signal<T> | [Store<T>, SetStoreFunction<T>], | ||
| O extends Record<string, any> | undefined, | ||
| T = SignalType<S>, | ||
| >( | ||
| signal: S, | ||
| options: PersistenceOptions<T, O> = {} as PersistenceOptions<T, O>, | ||
| ): PersistedState<S> { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate persisted.ts and inspect PersistenceOptions definition for context
fd persisted.ts packages/storage/src | xargs -I{} sed -n '1,120p' {}Repository: solidjs-community/solid-primitives
Length of output: 2020
🌐 Web query:
TypeScript infer type parameter from another type parameter constraint S extends Signal<T> inference from constraint only
💡 Result:
TypeScript does not support direct inference of a type parameter (e.g., T) solely from the constraint of another type parameter (e.g., S extends Signal) when invoking a function [1]. In TypeScript, type parameters are primarily inferred from the actual types of the function's arguments, not from the constraints defined on the generic parameters themselves [1]. Because constraints are used as checks rather than as inference sites, defining a function like function example<T, S extends Signal>(signal: S) {... } often results in T failing to be inferred correctly or defaulting to unknown [1]. To work around this limitation, you can use the following common techniques: 1. Indexed Access Types: If the type T can be extracted from the structure of S, use indexed access types instead of a secondary type parameter. For example, if Signal has a property that holds the value, you can define the type as S['value'] or create a utility type to extract it [2][3]. 2. Conditional Types with infer: Use a conditional type to extract the inner type T from the signal type S [4][5]. type GetSignalValue = S extends Signal? T: never; 3. Intersection Types at Usage: If you must rely on inference, you can sometimes improve it by using an intersection type in the function parameter to provide an additional "inference site" [1]. function example<S extends Signal>(signal: S & Signal) {... } 4. Avoid Dependent Generics: Where possible, refactor your code to remove the circularity or dependency between the generics, or accept that you may need to explicitly provide the type arguments when calling the function [6][1][3]. This behavior is a known limitation of the TypeScript compiler [1][7], and while various workarounds exist, no direct feature exists to infer generic constraints themselves as primary inference sources [1].
Citations:
- 1: https://stackoverflow.com/questions/76294709/why-cant-typescript-infer-generic-type-parameter-when-one-type-parameter-depend
- 2: https://stackoverflow.com/questions/74545039/typescript-infer-generic-types-in-generic-type-constraint
- 3: https://stackoverflow.com/questions/72897620/how-to-infer-in-typescript-template-parameters
- 4: https://2ality.com/2025/02/typescript-infer-operator.html
- 5: https://exploringjs.com/ts/book/ch_infer-keyword.html
- 6: Type inference for advanced
linkedSignalsignature angular/angular#60423 - 7: Infer declarations are allowed in generic constraints, but do not infer generic constraint types microsoft/TypeScript#41040
🌐 Web query:
TypeScript infer from generic constraint S extends Signal<T> example compiler behavior
💡 Result:
In TypeScript, the ability to infer a type from a generic constraint (such as S extends Signal) depends significantly on whether you are using the infer keyword within a conditional type or attempting to perform inference at the generic parameter level. ### Conditional Types and infer When you use infer inside a conditional type, it is specifically designed to extract parts of a type during type matching [1][2][3]. Since TypeScript 4.7, you can use the infer T extends Constraint syntax to combine inference and constraint checking in a single step [1]. Example: ts // Extract T from a Signal<T> type type GetSignalValue<S> = S extends Signal<infer T> ? T : never; // Using infer with an explicit constraint (TS 4.7+) type FirstString<T> = T extends [infer H extends string, ...any[]] ? H : never; This behavior is stable and well-supported [1]. ### Generic Constraints and Inference Limitations Attempting to infer type parameters directly from generic constraints (e.g., <S extends Signal<T>>) in function signatures or type definitions often encounters limitations because TypeScript does not automatically "reverse" generic constraints to infer the underlying type parameters (like T) [4][5]. 1. Inference Does Not Flow "Backward": When you define a generic type with a constraint like <S extends Signal<T>, T>, TypeScript often cannot automatically deduce T simply from the presence of S unless T is explicitly included and constrained in the signature [5]. 2. unknown Results: In complex signatures—particularly those involving callbacks or signals—TypeScript may default to unknown for inferred types because it cannot determine the specific type arguments without explicit help or simplified structural signatures [6][5]. 3. Compiler Workarounds: - Manual Specification: Forcing the user to specify both generics (e.g., <S, T>(signal: S & Signal<T>)) is the most reliable way to avoid unknown inference [5]. - NoInfer Utility: To control inference and prevent TypeScript from over-widening types, developers often use the NoInfer<T> utility type, which blocks inference for specific parts of a signature [6][7]. ### Summary of Compiler Behavior The compiler treats infer as a structural matching tool within conditional types [1][2]. However, generic constraints (e.g., S extends Signal<T>) are treated as bounds on existing types rather than sources for new inferences [4][5]. If you need to extract T from S, you should use a conditional type mapping (e.g., S extends Signal<infer T> ? T : never) rather than relying on the generic constraint definition itself to perform the extraction [1][2].
Citations:
- 1: https://blog.openreplay.com/infer-typescript/
- 2: https://2ality.com/2025/02/typescript-infer-operator.html
- 3: https://www.typescriptlang.org/docs/handbook/2/conditional-types.html
- 4: Infer declarations are allowed in generic constraints, but do not infer generic constraint types microsoft/TypeScript#41040
- 5: Inference does not extend to generics of types, causing widened covariant types, and errors for contravariant types microsoft/TypeScript#44328
- 6: Function Generic is
unknownif argument hasNoInfer<Generic>but returns the Generic microsoft/TypeScript#60922 - 7: Type inference for advanced
linkedSignalsignature angular/angular#60423
Infer T from signal, not from the S constraint.
T has no direct inference site here, so options.serialize / deserialize / sync can end up typed as unknown instead of the signal element type. Extract T from S in the signature if you want those callbacks checked against the actual value type.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/storage/src/persisted.ts` around lines 95 - 111, The makePersisted
overloads are not inferring T from the signal value, so PersistenceOptions
callbacks can fall back to unknown. Update the makePersisted signature in
persisted.ts to derive T from S using the signal/store element type instead of
the current generic constraint, and keep the overloads aligned so
options.serialize, deserialize, and sync are checked against the actual signal
type.
| const storage = options.storage || (globalThis.localStorage as Storage | undefined); | ||
| const name = options.name || `storage-${createUniqueId()}`; | ||
| if (!storage) { | ||
| return [signal[0], signal[1], null] as PersistedState<S>; | ||
| return Object.assign(signal, { 2: null }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
No-storage branch mutates the caller's signal in place, unlike the storage-available branch.
Object.assign(signal, { 2: null }) mutates the array passed in by the caller directly (growing .length from 2 to 3), whereas the storage-available path (Lines 161-182) builds a new array via Object.assign([], signal, {...}) without touching the original. If a caller holds a reference to the original signal tuple beyond destructuring (e.g. stores it, passes it elsewhere), that reference is now silently mutated with an unexpected third index only in the no-storage case.
♻️ Make both branches consistent by not mutating the input
if (!storage) {
- return Object.assign(signal, { 2: null });
+ return Object.assign([], signal, { 2: null }) as PersistedState<S>;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const storage = options.storage || (globalThis.localStorage as Storage | undefined); | |
| const name = options.name || `storage-${createUniqueId()}`; | |
| if (!storage) { | |
| return [signal[0], signal[1], null] as PersistedState<S>; | |
| return Object.assign(signal, { 2: null }); | |
| const storage = options.storage || (globalThis.localStorage as Storage | undefined); | |
| const name = options.name || `storage-${createUniqueId()}`; | |
| if (!storage) { | |
| return Object.assign([], signal, { 2: null }) as PersistedState<S>; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/storage/src/persisted.ts` around lines 112 - 115, The no-storage
branch in persisted storage is mutating the caller-provided signal tuple in
place via Object.assign(signal, { 2: null }), which makes it inconsistent with
the storage-available path in persisted.ts. Update the early return in the
persisted logic so it creates and returns a new tuple/array, matching the
cloning approach used later in the storage-enabled branch, and avoid writing to
the original signal object.
4 participants
docs(storage): resource usage
This simplifies the types considerably and corrects the documentation on how to use a persisted state with createResource.
Summary by CodeRabbit
Bug Fixes
Documentation