upgrade: storage package upgrade for Solid 2.0#851
Conversation
🦋 Changeset detectedLatest commit: 1c35ccf 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 |
…e and event-listener
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
# Conflicts: # README.md # pnpm-lock.yaml
Deploying solid-primitives with
|
| Latest commit: |
1c35ccf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://24232aef.solid-primitives.pages.dev |
| Branch Preview URL: | https://update-v2-storage.solid-primitives.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/storage/src/persisted.ts (1)
167-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix inverted URL-sync filter and prevent server
location.hrefcrashesIn
packages/storage/src/persisted.ts, the URL skip condition inside the sync callback is inverted:
- On the client,
globalThis.windowis truthy, so!globalThis.windowisfalseand the URL comparison never runs.- On the server,
!globalThis.windowistrue, andglobalThis.location.hrefis still evaluated (RHS of!==), which can throwTypeErrorwhenlocationis missing.if ( data.key !== name || (!globalThis.window && (data.url || globalThis.location.href) !== globalThis.location.href) || data.newValue === serialize(untrack(get)) ) { return; }Possible intent
- (!globalThis.window && (data.url || globalThis.location.href) !== globalThis.location.href) || + (!!globalThis.window && !!data.url && data.url !== globalThis.location.href) ||🤖 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 167 - 173, The URL check is inverted and currently reads location.href on the server, causing crashes; change the condition so the URL comparison only runs when running in a browser (globalThis.window is truthy) and avoid accessing globalThis.location when window is false. Update the clause around data.url/location to be executed only when globalThis.window is true (e.g., guard before comparing or use optional chaining) so you compare (data.url || globalThis.location.href) against the current location only in the client, while keeping the existing checks for data.key and data.newValue === serialize(untrack(get)).
🧹 Nitpick comments (5)
site/src/routes/playground/$name.tsx~next (2)
58-64: ⚡ Quick winConsider adding an error boundary for module load failures.
If a lazy-loaded module fails to load or throws during render, the entire page will crash. Wrapping the
Dynamiccomponent in anErrorBoundarywould provide a better user experience.🚨 Example error boundary wrapper
+import { ErrorBoundary } from "solid-js"; + ... <div class="package-playground"> <ClientOnly> <Suspense> - <Dynamic component={modules[params().name]} /> + <ErrorBoundary fallback={(err) => <div>Failed to load playground: {err.message}</div>}> + <Dynamic component={modules[params().name]} /> + </ErrorBoundary> </Suspense> </ClientOnly> </div>🤖 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 `@site/src/routes/playground/`$name.tsx~next around lines 58 - 64, The Dynamic lazy-loaded component can throw during load/render (Dynamic, modules[params().name]) so wrap it in an ErrorBoundary around the existing ClientOnly/Suspense placement to catch module load failures and render a user-friendly fallback; update the wrapper to display an error UI (and optionally a retry action) and log the error (e.g., via console or app logger) so failures of Dynamic do not crash the whole page.
18-19: ⚡ Quick winAdd defensive check for missing package name.
The
.at(-3)extraction assumes a fixed path structure (packages/*/dev/index.tsx). If the glob pattern or directory structure changes,namecould beundefined, leading to an entry withundefinedkey in the map.🛡️ Proposed defensive check
(acc, [path, fn]) => { const name = path.split("/").at(-3); - if (name) acc[name] = lazy(fn as any); + if (name && name !== "packages") acc[name] = lazy(fn as any); return acc; },🤖 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 `@site/src/routes/playground/`$name.tsx~next around lines 18 - 19, The reducer that builds the map uses path.split("/").at(-3) to compute name and can yield undefined; update the reducer (the (acc, [path, fn]) => { ... } block) to defensively handle a missing name by checking if name is falsy before using it—either skip adding the entry (and optionally log or collect a warning) or derive a safe fallback key; ensure you do not insert an undefined key into the resulting map and reference the exact extraction expression path.split("/").at(-3) when applying the guard.site/src/routes/playground/$name.tsx~port to tanstack start v1 (2)
18-19: ⚡ Quick winAdd defensive check for missing package name.
The
.at(-3)extraction assumes a fixed path structure (packages/*/dev/index.tsx). If the glob pattern or directory structure changes,namecould beundefined, leading to an entry withundefinedkey in the map.🛡️ Proposed defensive check
(acc, [path, fn]) => { const name = path.split("/").at(-3); - if (name) acc[name] = lazy(fn as any); + if (name && name !== "packages") acc[name] = lazy(fn as any); return acc; },🤖 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 `@site/src/routes/playground/`$name.tsx~port to tanstack start v1 around lines 18 - 19, The extraction of the package name via name = path.split("/").at(-3) can yield undefined and cause acc[undefined] entries; update the loop that sets acc[name] = lazy(fn as any) to defensively check the result of path.split(...).at(-3) (the name variable) and skip adding to acc when name is falsy (or log/throw a clear error) so only valid string keys are inserted; look for the occurrence of name, path.split("/").at(-3), and acc[name] = lazy(fn as any) to apply the guard.
42-48: ⚡ Quick winConsider adding an error boundary for module load failures.
If a lazy-loaded module fails to load or throws during render, the entire page will crash. Wrapping the
Dynamiccomponent in anErrorBoundarywould provide a better user experience.🚨 Example error boundary wrapper
+import { ErrorBoundary } from "solid-js"; + ... <div class="package-playground"> <ClientOnly> <Suspense> - <Dynamic component={modules[params().name]} /> + <ErrorBoundary fallback={(err) => <div>Failed to load playground: {err.message}</div>}> + <Dynamic component={modules[params().name]} /> + </ErrorBoundary> </Suspense> </ClientOnly> </div>🤖 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 `@site/src/routes/playground/`$name.tsx~port to tanstack start v1 around lines 42 - 48, Wrap the lazy-loaded Dynamic render in an ErrorBoundary so module load/render failures don’t crash the page: create or reuse an ErrorBoundary component and place it around <Dynamic component={modules[params().name]} /> (inside the existing ClientOnly and Suspense), providing a fallback UI and optional retry logic; ensure the boundary catches errors thrown by Dynamic (and by modules[params().name]) and surfaces a user-friendly message instead of letting the whole page fail.packages/storage/src/persisted.ts (1)
127-130: 💤 Low valueIn-place mutation of the input signal tuple.
(signal as any)[1] = action(actionFn)mutates the caller-provided tuple. It works for the inline-array call style, but it's a surprising side effect on the argument. Consider deriving a local wrapped setter instead of overwritingsignal[1], and use that local in the returned tuple/setlogic.🤖 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 127 - 130, The code mutates the caller-provided tuple by assigning (signal as any)[1] = action(actionFn); instead, capture actionFn = options.action && options.action(signal) and create a local wrapped setter (e.g., const wrappedSet = action(actionFn)) and use that local wherever the setter is needed (in the returned tuple or in set logic) rather than overwriting signal[1]; if returning a tuple that must include the wrapped setter, construct and return a new tuple/array using signal[0] and wrappedSet so the original signal argument is not mutated.
🤖 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 1-2: The import list currently pulls reconcile and snapshot from
"solid-js" but those APIs are exported from "solid-js/store"; update the module
imports so that reconcile and snapshot are imported from "solid-js/store" while
keeping Signal, StoreSetter, Store and the other runtime helpers (action,
createUniqueId, latest, untrack, DEV) from "solid-js" as appropriate; adjust the
import statements to reference reconcile and snapshot from "solid-js/store"
(look for uses of reconcile and snapshot in this file to verify no other changes
are needed).
- Around line 149-157: The persisted storage setter passes an invalid second
argument to solid-js reconcile—replace reconcile(value, () => undefined) with a
valid call (either remove the second argument and use reconcile(value) or
provide a proper ReconcileOptions object like { key?, merge? }) so the reconcile
API is used correctly; update the setter where deserialize(...) is reconciled
and the state is set via (signal[1] as any) to use the corrected reconcile
invocation.
In `@site/src/routes/package/`$name/index.tsx~port to tanstack start v1:
- Around line 14-20: The Route definition for createFileRoute("/package/$name/")
currently calls fetchPackageData(params.name) directly in loader and lacks a
notFoundComponent; update the Route to supply notFoundComponent: NotFound
(imported from ~/components/NotFound) and change the loader to try/catch around
fetchPackageData(params.name) so that any rejection or missing data results in
throwing notFound() from the catch branch; touch the Route export, loader
function, and the notFoundComponent property to implement this behavior (refer
to Route, loader, fetchPackageData, notFoundComponent, NotFound, and notFound).
In `@site/src/routes/playground/`$name.tsx~next:
- Line 19: The code uses import.meta.glob(...) loaders and then calls lazy(fn as
any) which hides a type mismatch; change the loader passed to Solid's lazy() so
it returns a Promise resolving to { default: ... } instead of asserting any —
for example, adapt each globbed fn by wrapping it like fn().then(m => ({
default: m.default })) or change the import.meta.glob call to use the
appropriate import option (e.g., import: 'default'), then remove the `as any`
and pass the correctly-typed loader into lazy() (referencing the `lazy()` call
and the glob-generated `fn`/`modules` construction).
In `@site/src/routes/playground/`$name.tsx~port to tanstack start v1:
- Around line 25-30: Add a loader to the createFileRoute configuration that
validates params.name against the available modules before rendering: inside the
Route created by createFileRoute("/playground/$name") (where PlaygroundPage
reads modules[params().name] and the Dynamic component expects a valid module),
implement a loader that reads params.name, checks modules[params.name] (or the
package registry you use) and if missing returns a 404 Response or throws a
redirect to a not-found route; otherwise return the validated name (or module)
in the loader data so PlaygroundPage consumes a guaranteed-valid value and
avoids passing undefined to Dynamic.
- Line 19: The current use of lazy(fn as any) can pass modules with no default
export (e.g., packages/list-state/dev/index.tsx and packages/sse/dev/index.tsx),
causing Solid's lazy loader to fail; either update those dev entry files to
export a default component (ensure packages/list-state/dev/index.tsx and
packages/sse/dev/index.tsx export default the React/Solid component) or
transform the imported module before calling lazy (map the resolved module to an
object with a default property, e.g., wrap the module so modules[name] becomes {
default: Component }) and then pass that to lazy instead of using the unsafe
cast in lazy(fn as any).
---
Outside diff comments:
In `@packages/storage/src/persisted.ts`:
- Around line 167-173: The URL check is inverted and currently reads
location.href on the server, causing crashes; change the condition so the URL
comparison only runs when running in a browser (globalThis.window is truthy) and
avoid accessing globalThis.location when window is false. Update the clause
around data.url/location to be executed only when globalThis.window is true
(e.g., guard before comparing or use optional chaining) so you compare (data.url
|| globalThis.location.href) against the current location only in the client,
while keeping the existing checks for data.key and data.newValue ===
serialize(untrack(get)).
---
Nitpick comments:
In `@packages/storage/src/persisted.ts`:
- Around line 127-130: The code mutates the caller-provided tuple by assigning
(signal as any)[1] = action(actionFn); instead, capture actionFn =
options.action && options.action(signal) and create a local wrapped setter
(e.g., const wrappedSet = action(actionFn)) and use that local wherever the
setter is needed (in the returned tuple or in set logic) rather than overwriting
signal[1]; if returning a tuple that must include the wrapped setter, construct
and return a new tuple/array using signal[0] and wrappedSet so the original
signal argument is not mutated.
In `@site/src/routes/playground/`$name.tsx~next:
- Around line 58-64: The Dynamic lazy-loaded component can throw during
load/render (Dynamic, modules[params().name]) so wrap it in an ErrorBoundary
around the existing ClientOnly/Suspense placement to catch module load failures
and render a user-friendly fallback; update the wrapper to display an error UI
(and optionally a retry action) and log the error (e.g., via console or app
logger) so failures of Dynamic do not crash the whole page.
- Around line 18-19: The reducer that builds the map uses path.split("/").at(-3)
to compute name and can yield undefined; update the reducer (the (acc, [path,
fn]) => { ... } block) to defensively handle a missing name by checking if name
is falsy before using it—either skip adding the entry (and optionally log or
collect a warning) or derive a safe fallback key; ensure you do not insert an
undefined key into the resulting map and reference the exact extraction
expression path.split("/").at(-3) when applying the guard.
In `@site/src/routes/playground/`$name.tsx~port to tanstack start v1:
- Around line 18-19: The extraction of the package name via name =
path.split("/").at(-3) can yield undefined and cause acc[undefined] entries;
update the loop that sets acc[name] = lazy(fn as any) to defensively check the
result of path.split(...).at(-3) (the name variable) and skip adding to acc when
name is falsy (or log/throw a clear error) so only valid string keys are
inserted; look for the occurrence of name, path.split("/").at(-3), and acc[name]
= lazy(fn as any) to apply the guard.
- Around line 42-48: Wrap the lazy-loaded Dynamic render in an ErrorBoundary so
module load/render failures don’t crash the page: create or reuse an
ErrorBoundary component and place it around <Dynamic
component={modules[params().name]} /> (inside the existing ClientOnly and
Suspense), providing a fallback UI and optional retry logic; ensure the boundary
catches errors thrown by Dynamic (and by modules[params().name]) and surfaces a
user-friendly message instead of letting the whole page fail.
🪄 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: e470d719-ebaa-45df-a2eb-d7063275920f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.changeset/eleven-buttons-jump.mdpackages/storage/package.jsonpackages/storage/src/cookies.tspackages/storage/src/persisted.tspackages/storage/tauri-storage/package.jsonpackages/storage/test/persisted.test.tssite/src/routes/package/$name/-components/primitive-name-tooltips.tsx~nextsite/src/routes/package/$name/-components/primitive-name-tooltips.tsx~port to tanstack start v1site/src/routes/package/$name/index.tsx~nextsite/src/routes/package/$name/index.tsx~port to tanstack start v1site/src/routes/playground/$name.tsx~nextsite/src/routes/playground/$name.tsx~port to tanstack start v1
| import type { Signal, StoreSetter, Store } from "solid-js"; | ||
| import { action, createUniqueId, latest, untrack, reconcile, snapshot, DEV } from "solid-js"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does solid-js 2.0.0-beta.14 export reconcile and snapshot from the root "solid-js" package entry?
💡 Result:
No, solid-js version 2.0.0-beta.14 does not export reconcile or snapshot from the root solid-js package entry [1][2][3]. In SolidJS, reconcile and snapshot are utilities associated with the store system and are exported from the solid-js/store module [2][3]. While Solid 2.0 involves significant changes to the package structure—including the removal of explicit solid-js/web and solid-js/store subpaths in favor of a consolidated approach in some contexts—these specific store-related utilities continue to reside within the store-specific API surface, not the root export [1][2]. Developers should continue to import these from solid-js/store [2][3].
Citations:
- 1: https://npmx.dev/package/solid-js/v/2.0.0-beta.14
- 2: https://docs.solidjs.com/concepts/stores
- 3: https://github.com/solidjs-community/solid-primitives/blob/main/packages/history/dev/index.tsx
Fix incorrect imports of reconcile/snapshot from the Solid root
packages/storage/src/persisted.ts imports reconcile and snapshot from "solid-js" (lines 1-2), but solid-js@2.0.0-beta.14 does not export reconcile/snapshot from the package root—these are store-related APIs exposed from solid-js/store. Update the imports to use the correct module to avoid build-time resolution errors.
🤖 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 1 - 2, The import list
currently pulls reconcile and snapshot from "solid-js" but those APIs are
exported from "solid-js/store"; update the module imports so that reconcile and
snapshot are imported from "solid-js/store" while keeping Signal, StoreSetter,
Store and the other runtime helpers (action, createUniqueId, latest, untrack,
DEV) from "solid-js" as appropriate; adjust the import statements to reference
reconcile and snapshot from "solid-js/store" (look for uses of reconcile and
snapshot in this file to verify no other changes are needed).
| : (data: string) => { | ||
| try { | ||
| const value = deserialize(data); | ||
| (signal[1] as any)(reconcile(value)); | ||
| (signal[1] as any)(reconcile(value, () => undefined)); | ||
| } catch (e) { | ||
| // eslint-disable-next-line no-console | ||
| if (isDev) console.warn(e); | ||
| if (DEV) console.warn(e); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the second argument signature of reconcile in solid-js 2.0 (ReconcileOptions)?
💡 Result:
In SolidJS 2.0, the reconcile function accepts a second, optional argument that is an options object [1]. The signature for this options object is as follows [1]: { key?: string | null; merge?: boolean; } The key property specifies the property name used to match items in arrays during reconciliation (e.g., "id"), while the merge property determines whether reconciliation pushes updates deeper into the structure (true) or prefers replacing non-matching branches (false) [1].
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '120,190p' packages/storage/src/persisted.ts | nl -baRepository: solidjs-community/solid-primitives
Length of output: 121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== persisted.ts (approx lines 130-180) ==="
sed -n '130,180p' packages/storage/src/persisted.ts | awk '{printf "%5d %s\n", NR+129, $0}'
echo
echo "=== reconcile usages in persisted.ts ==="
rg -n "reconcile\\(" packages/storage/src/persisted.ts
echo
echo "=== reconcile imports/typing in persisted.ts ==="
rg -n "import .*reconcile|from .*reconcile|type .*Reconcile|ReconcileOptions" packages/storage/src/persisted.tsRepository: solidjs-community/solid-primitives
Length of output: 2573
🌐 Web query:
solid-js reconcile(value, options) implementation expects ReconcileOptions { key?, merge? } what happens if options is a function
💡 Result:
In SolidJS, the reconcile store utility officially expects the options argument to be an object with the properties key (string | null) and merge (boolean) [1]. It does not support receiving a function as the options argument [1]. If you pass a function where an options object is expected, the implementation will attempt to access the key and merge properties directly on that function (e.g., function.key, function.merge). Since these properties are typically undefined on a function object, they will be treated as their default values, effectively ignoring the function and proceeding with default reconciliation behavior [1]. It is worth noting that some third-party libraries, such as TanStack Query, have introduced their own custom reconcile options or patterns that allow providing a function to define custom reconciliation logic (e.g., (oldData, newData) => reconciledData) [2]. However, this is specific to those libraries' implementations and is distinct from the official solid-js/store reconcile utility [1][2].
Citations:
- 1: https://docs.solidjs.com/reference/store-utilities/reconcile
- 2: feat(solid-query): Add
reconcileoption TanStack/query#5287
Fix reconcile second argument type in persisted storage setter
reconcile from solid-js expects its second argument to be a ReconcileOptions object ({ key?, merge? }). Passing () => undefined is an unsupported value and won’t apply meaningful options (its .key/.merge will be read as undefined, so defaults are used). Remove the second argument or pass real options.
Proposed change
- (signal[1] as any)(reconcile(value, () => undefined));
+ (signal[1] as any)(reconcile(value));📝 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.
| : (data: string) => { | |
| try { | |
| const value = deserialize(data); | |
| (signal[1] as any)(reconcile(value)); | |
| (signal[1] as any)(reconcile(value, () => undefined)); | |
| } catch (e) { | |
| // eslint-disable-next-line no-console | |
| if (isDev) console.warn(e); | |
| if (DEV) console.warn(e); | |
| } | |
| }; | |
| : (data: string) => { | |
| try { | |
| const value = deserialize(data); | |
| (signal[1] as any)(reconcile(value)); | |
| } catch (e) { | |
| // eslint-disable-next-line no-console | |
| if (DEV) console.warn(e); | |
| } | |
| }; |
🤖 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 149 - 157, The persisted
storage setter passes an invalid second argument to solid-js reconcile—replace
reconcile(value, () => undefined) with a valid call (either remove the second
argument and use reconcile(value) or provide a proper ReconcileOptions object
like { key?, merge? }) so the reconcile API is used correctly; update the setter
where deserialize(...) is reconciled and the state is set via (signal[1] as any)
to use the corrected reconcile invocation.
| export const Route = createFileRoute("/package/$name/")({ | ||
| component: PackagePage, | ||
| loader: async ({ params }) => fetchPackageData(params.name), | ||
| head: ({ params }) => ({ | ||
| meta: [{ title: `${kebabCaseToCapitalized(params.name)} — Solid Primitives` }], | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for root-level notFound/error boundaries and existing notFound usage in routes
rg -nP --type=ts --type=tsx -C3 '\b(notFoundComponent|errorComponent|notFound)\b' site/src/routes
fd -i 'NotFound' site/srcRepository: solidjs-community/solid-primitives
Length of output: 157
Loader failures in this route should render a 404: the route loader currently just calls fetchPackageData(params.name) without converting rejections into notFound() (and it doesn’t provide notFoundComponent); moreover, site/src/routes has no notFoundComponent, errorComponent, or notFound usage, so unknown package names will likely hit the generic router error path. Add notFoundComponent using ~/components/NotFound and wrap the loader in try/catch that throw notFound().
🤖 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 `@site/src/routes/package/`$name/index.tsx~port to tanstack start v1 around
lines 14 - 20, The Route definition for createFileRoute("/package/$name/")
currently calls fetchPackageData(params.name) directly in loader and lacks a
notFoundComponent; update the Route to supply notFoundComponent: NotFound
(imported from ~/components/NotFound) and change the loader to try/catch around
fetchPackageData(params.name) so that any rejection or missing data results in
throwing notFound() from the catch branch; touch the Route export, loader
function, and the notFoundComponent property to implement this behavior (refer
to Route, loader, fetchPackageData, notFoundComponent, NotFound, and notFound).
| ]), | ||
| ).reduce( | ||
| (acc, [path, fn]) => { | ||
| const name = path.split("/").at(-3); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Vite's import.meta.glob return type match the function signature expected by Solid's lazy() helper?
💡 Result:
Vite's import.meta.glob does not automatically return a type that matches the signature expected by Solid's lazy helper [1][2]. Solid's lazy helper expects a function that returns a Promise resolving to an object containing a default export: => Promise<{ default: T }> [2]. By default, import.meta.glob returns a record where each value is a function that returns a Promise resolving to the entire module namespace object (the "module" itself), not necessarily a { default:... } structure [1]. To use import.meta.glob with lazy, you must typically adapt the import to match the expected structure, for example: const MyComponent = lazy( => import('./MyComponent.tsx').then(m => ({ default: m.MyComponent }))); When using glob imports, this adaptation often involves explicitly extracting the component from the module record: const modules = import.meta.glob('./components/*.tsx'); const MyLazyComponent = lazy( => modules['./components/MyComponent.tsx'].then(m => ({ default: m.default }))); If your components use default exports, they may be more directly compatible, but you still need to ensure the return type of the glob-imported function matches the Promise<{ default: T }> contract expected by lazy. Use of the import option in import.meta.glob (e.g., { import: 'default' }) can help if you only need the default export, but you must still wrap the result to satisfy the Promise expectation if the return type of the glob function does not align [1][3].
Citations:
- 1: https://github.com/vitejs/vite/blob/130ef31b/packages/vite/types/importGlob.d.ts
- 2: https://docs.solidjs.com/reference/component-apis/lazy
- 3: https://vite.dev/guide/features
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the target file(s) matching the provided path
fd -t f '^\$name\.tsx~next$' site/src/routes/playground || true
# Also list any similar filename containing "$name.tsx~next"
fd -t f 'name\.tsx~next' site/src/routes/playground || trueRepository: solidjs-community/solid-primitives
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la site/src/routes/playground || true
# Show exact matches
fd -t f '[$]name\.tsx~next$' site/src/routes/playground || true
# Fallback: print all files in directory containing "tsx~next"
fd -t f '*tsx~next*' site/src/routes/playground || trueRepository: solidjs-community/solid-primitives
Length of output: 814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la site/src/routes/playground || true
fd -t f '*tsx~next*' site/src/routes/playground || true
fd -t f '[$]name\.tsx~next$' site/src/routes/playground || trueRepository: solidjs-community/solid-primitives
Length of output: 814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate file by name within repo
fd -t f '*$name.tsx~next*' . || true
fd -t f '[$]name\.tsx~next$' . || trueRepository: solidjs-community/solid-primitives
Length of output: 436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target_dir='site/src/routes/playground'
ls -la "$target_dir" || true
echo '--- matches ---'
fd -t f '\$name\.tsx~next$' "$target_dir" || true
echo '--- contains ---'
fd -t f '*tsx~next*' "$target_dir" || trueRepository: solidjs-community/solid-primitives
Length of output: 847
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='site/src/routes/playground/$name.tsx~next'
if [ -f "$file" ]; then
echo "FOUND: $file"
echo '--- lazy/as any context ---'
rg -n 'lazy\\(|as any|import\\.meta\\.glob' "$file" || true
echo '--- lines 1-120 (numbered) ---'
nl -ba "$file" | sed -n '1,140p'
else
echo "MISSING: $file"
fd -t f '[$]name\.tsx~next$' site/src/routes/playground || true
fiRepository: solidjs-community/solid-primitives
Length of output: 340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='site/src/routes/playground/\$name.tsx~next'
if [ -f "$file" ]; then
echo "FOUND: $file"
echo '--- lazy/as any context ---'
rg -n 'lazy\\(|as any|import\\.meta\\.glob' "$file" || true
echo '--- lines 1-200 (numbered) ---'
nl -ba "$file" | sed -n '1,200p'
else
echo "MISSING: $file"
fd -t f '\$name\.tsx~next$' site/src/routes/playground || true
fiRepository: solidjs-community/solid-primitives
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Also inspect any similarly named file in the folder in case globbing differs
fd -t f '\$name\.tsx~next$' site/src/routes/playground | while read -r f; do
echo "FOUND: $f"
rg -n 'lazy\\(|as any|import\\.meta\\.glob' "$f" || true
echo '--- lines 1-200 (numbered) ---'
nl -ba "$f" | sed -n '1,200p'
doneRepository: solidjs-community/solid-primitives
Length of output: 312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file that review comment refers to
candidates=$(fd -t f '\$name\.tsx~next$' site/src/routes/playground || true)
if [ -z "${candidates:-}" ]; then
echo "No exact match for site/src/routes/playground/\\$name.tsx~next"
echo "--- other *tsx~next* files ---"
fd -t f '*tsx~next*' site/src/routes/playground || true
exit 0
fi
# Print contexts for each candidate
echo "$candidates" | while read -r f; do
echo "=== FILE: $f ==="
rg -n 'lazy\\(|as any|import\\.meta\\.glob' "$f" || true
echo '--- excerpt around glob/lazy (lines 1-220) ---'
nl -ba "$f" | sed -n '1,220p'
doneRepository: solidjs-community/solid-primitives
Length of output: 336
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='site/src/routes/playground/$name.tsx~next'
echo '--- first 220 lines ---'
sed -n '1,220p' "$file" || true
echo '--- lines containing exact strings ---'
rg -nF 'lazy(' "$file" || true
rg -nF 'import.meta.glob' "$file" || true
rg -nF 'as any' "$file" || true
echo '--- more context around import.meta.glob ---'
# Try to locate the closest block by printing a wider window around matches if present
line_nums=$(rg -nF 'import.meta.glob' "$file" | cut -d: -f1 || true)
if [ -n "${line_nums:-}" ]; then
for ln in $line_nums; do
start=$((ln-40)); end=$((ln+60))
if [ $start -lt 1 ]; then start=1; fi
echo "### window around import.meta.glob at line $ln ($start-$end)"
sed -n "${start},${end}p" "$file"
done
fiRepository: solidjs-community/solid-primitives
Length of output: 4563
Fix lazy() loader typing for import.meta.glob (remove/justify as any)
site/src/routes/playground/$name.tsx~next builds modules from import.meta.glob([...]) and then does acc[name] = lazy(fn as any). With no import: 'default' option, Vite’s glob loader typically resolves to the module namespace object, while Solid’s lazy() expects a loader returning Promise<{ default: T }> (so the as any can hide a real type/shape mismatch). Prefer adapting the glob result to { default: ... } (e.g., wrap fn().then(m => ({ default: m.default })) / use the appropriate import option) and then drop the as any.
🤖 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 `@site/src/routes/playground/`$name.tsx~next at line 19, The code uses
import.meta.glob(...) loaders and then calls lazy(fn as any) which hides a type
mismatch; change the loader passed to Solid's lazy() so it returns a Promise
resolving to { default: ... } instead of asserting any — for example, adapt each
globbed fn by wrapping it like fn().then(m => ({ default: m.default })) or
change the import.meta.glob call to use the appropriate import option (e.g.,
import: 'default'), then remove the `as any` and pass the correctly-typed loader
into lazy() (referencing the `lazy()` call and the glob-generated `fn`/`modules`
construction).
| ).reduce( | ||
| (acc, [path, fn]) => { | ||
| const name = path.split("/").at(-3); | ||
| if (name) acc[name] = lazy(fn as any); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Vite's import.meta.glob return type match the function signature expected by Solid's lazy() helper?
💡 Result:
Yes—when you use Vite’s non-eager (lazy) form, the return type of each glob entry matches the function signature that Solid’s lazy expects. - Vite typing: import.meta.glob(pattern) (with eager defaulting to false) produces an object whose values are functions returning a Promise of the module type: Record<string, => Promise>. [1] - Solid typing: lazy is declared to take a function that returns Promise<{ default: T }>. [2] So if your Vite glob is importing components as default exports (e.g., import.meta.glob('./*.tsx') where T’s shape includes default), then each value function has the exact shape => Promise<{ default: T }>, which is what lazy requires. If you use Vite’s eager mode (import.meta.glob(pattern, { eager: true })), the values are not functions returning promises; they are synchronous module values (Record<string, T>), so they do not match Solid’s lazy signature. [1] Practical consequence: use Vite’s default (non-eager) glob and ensure the imported module’s component is the default export (or map it to { default: Component }), then you can pass the per-file loader function directly to Solid’s lazy. [2][1]
Citations:
- 1: feat!: rework
import.meta.globvitejs/vite#7537 - 2: https://www.reddit.com/r/solidjs/comments/1izk8e4/why_doesnt_glob_imports_work_in_solid/
🌐 Web query:
Solid lazy TypeScript signature: what function type does lazy() accept?
💡 Result:
The SolidJS lazy function accepts a single argument: a function that returns a Promise which resolves to an object containing the component as its default export [1][2]. Specifically, the function type accepted by lazy is [1][2]: => Promise<{ default: T }> In this signature, T represents the type of the component being imported (where T extends Component). When you use lazy, the import statement must resolve to a module where the component is assigned to the default export property [1][3]. If your module does not use a default export for the component, you must manually map the import to an object with a default property [3]: const MyComponent = lazy( => import('./MyComponent').then(m => ({ default: m.MyComponent }))); The lazy function returns a component that is compatible with the original component type T, but also includes an additional preload method, which returns the same Promise to allow for preloading the component chunk before it is needed [1][2].
Citations:
- 1: https://docs.solidjs.com/reference/component-apis/lazy
- 2: https://github.com/solidjs/solid/blob/7d913a69/packages/solid/src/server/rendering.ts
- 3: Lazy to support non defaults solidjs/solid#1030
🌐 Web query:
Vite TypeScript types for import.meta.glob return type (eager vs non-eager) what is the value type?
💡 Result:
In Vite, the return type of import.meta.glob depends on the eager option [1]. When eager is set to true ({ eager: true }), the function returns a Record where the keys are file paths (as strings) and the values are the imported modules themselves [1]. The type of the value is the module content directly [1]. When eager is false (or omitted, as the default is false), the function returns a Record where the keys are file paths (as strings) and the values are functions that return a Promise resolving to the module [1]. In TypeScript, this is defined by the following logic [1]: - If eager is true: Record<string, T> [1] - If eager is false: Record<string, => Promise> [1] Here, T represents the type of the imported module (which defaults to unknown unless a generic type is provided) [1]. To ensure these types are available in your TypeScript project, ensure your tsconfig.json includes "vite/client" in the compilerOptions.types array [2].
Citations:
- 1: https://github.com/vitejs/vite/blob/130ef31b/packages/vite/types/importGlob.d.ts
- 2: https://stackoverflow.com/questions/76973229/cannot-set-options-on-vite-glob-import
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the target file
fd -t f '^\$name\.tsx~port to tanstack start v1$' -a . || true
# If the exact filename match fails, print close matches
fd -t f '*playground*' site/src/routes -a | rg '\$name\.tsx' || trueRepository: solidjs-community/solid-primitives
Length of output: 470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant portion around line 19
FILE="$(fd -t f '*playground*' site/src/routes -a | rg '\$name\.tsx' | head -n 1 || true)"
echo "Using file: $FILE"
if [ -z "${FILE:-}" ]; then
exit 1
fi
nl -ba "$FILE" | sed -n '1,120p'Repository: solidjs-community/solid-primitives
Length of output: 401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='site/src/routes/playground/$name.tsx~port to tanstack start v1'
if [ ! -f "$FILE" ]; then
echo "File not found: $FILE" >&2
exit 1
fi
nl -ba "$FILE" | sed -n '1,120p'Repository: solidjs-community/solid-primitives
Length of output: 122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='site/src/routes/playground/$name.tsx~port to tanstack start v1'
if [ ! -f "$FILE" ]; then
echo "File not found: $FILE" >&2
exit 1
fi
# Show with line numbers (cat -n)
cat -n "$FILE" | sed -n '1,120p'Repository: solidjs-community/solid-primitives
Length of output: 2000
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List a few dev index.tsx files
fd -t f "packages/.*/dev/index\\.tsx$" -a . | head -n 20
# For up to 5, show their exports (first ~80 lines)
count=0
while IFS= read -r f; do
[ -z "$f" ] && continue
count=$((count+1))
echo "---- $f (showing first 120 lines) ----"
cat -n "$f" | sed -n '1,120p'
[ "$count" -ge 5 ] && break
done < <(fd -t f "packages/.*/dev/index\\.tsx$" -a .)Repository: solidjs-community/solid-primitives
Length of output: 136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Pick first 5 dev index.tsx files
mapfile -t files < <(fd -t f "packages/.*/dev/index\\.tsx$" -a . | head -n 5)
if [ "${`#files`[@]}" -eq 0 ]; then
echo "No matching packages/*/dev/index.tsx found" >&2
exit 1
fi
for f in "${files[@]}"; do
echo "---- $f (showing first 120 lines) ----"
cat -n "$f" | sed -n '1,120p'
doneRepository: solidjs-community/solid-primitives
Length of output: 135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TMP_LIST="/tmp/dev_index_files.txt"
rm -f "$TMP_LIST"
fd -t f "packages/.*/dev/index\\.tsx$" -a . | head -n 5 > "$TMP_LIST" || true
if [ ! -s "$TMP_LIST" ]; then
echo "No matching packages/*/dev/index.tsx found" >&2
exit 1
fi
i=0
while IFS= read -r f; do
[ -z "$f" ] && continue
i=$((i+1))
echo "---- $f (showing first 120 lines) ----"
cat -n "$f" | sed -n '1,120p'
done < "$TMP_LIST"
echo "Inspected $i file(s)."Repository: solidjs-community/solid-primitives
Length of output: 121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect packages directory
ls -la packages | head -n 50
echo "---- find packages/*/dev/index.tsx ----"
find packages -path '*/dev/index.tsx' -print | head -n 20
echo "---- sample of dev directories ----"
find packages -maxdepth 3 -type d -name dev -print | head -n 20Repository: solidjs-community/solid-primitives
Length of output: 4244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Count default exports in dev index files
echo "Default exports in packages/*/dev/index.tsx:"
rg -n "export default" packages/*/dev/index.tsx | head -n 50
echo
echo "Named exports (non-default) in packages/*/dev/index.tsx (sample):"
rg -n "export (const|function|class) " packages/*/dev/index.tsx | head -n 50Repository: solidjs-community/solid-primitives
Length of output: 3157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TMP="/tmp/dev_index_files.txt"
find packages -path "*/dev/index.tsx" -print > "$TMP"
total=$(wc -l < "$TMP" | tr -d ' ')
echo "Found $total packages/*/dev/index.tsx"
missing=0
echo "Missing 'export default' in:"
while IFS= read -r f; do
if ! rg -q "export default" "$f"; then
missing=$((missing+1))
echo " - $f"
fi
done < "$TMP"
echo "Missing export default count: $missing"Repository: solidjs-community/solid-primitives
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in packages/list-state/dev/index.tsx packages/sse/dev/index.tsx; do
echo "---- $f ----"
cat -n "$f" | sed -n '1,220p'
doneRepository: solidjs-community/solid-primitives
Length of output: 5790
Fix lazy(fn as any)—some dev modules don’t default-export a component
Solid’s lazy() loader must resolve to { default: ... }, but the current as any hides that mismatch: packages/list-state/dev/index.tsx and packages/sse/dev/index.tsx don’t export default (only named exports in list-state, and a non-default App that directly calls render() in sse). As a result, modules[name] will be wired to lazy() with no default export for those entries.
- Update those dev entry files to
export defaulta component, or map the glob module to{ default: Component }before passing it tolazy().
🤖 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 `@site/src/routes/playground/`$name.tsx~port to tanstack start v1 at line 19,
The current use of lazy(fn as any) can pass modules with no default export
(e.g., packages/list-state/dev/index.tsx and packages/sse/dev/index.tsx),
causing Solid's lazy loader to fail; either update those dev entry files to
export a default component (ensure packages/list-state/dev/index.tsx and
packages/sse/dev/index.tsx export default the React/Solid component) or
transform the imported module before calling lazy (map the resolved module to an
object with a default property, e.g., wrap the module so modules[name] becomes {
default: Component }) and then pass that to lazy instead of using the unsafe
cast in lazy(fn as any).
| export const Route = createFileRoute("/playground/$name")({ | ||
| component: PlaygroundPage, | ||
| head: ({ params }) => ({ | ||
| meta: [{ title: `${kebabCaseToCapitalized(params.name)} — Playground` }], | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Add loader validation to prevent runtime errors.
This route configuration lacks validation of the params.name parameter. When a user navigates to an invalid package name (e.g., /playground/nonexistent), the code will access modules[params().name] at line 45 and pass undefined to the Dynamic component, causing a runtime error.
🔒 Proposed fix: add loader with validation
+import { notFound } from "`@tanstack/solid-router`";
+import NotFound from "~/components/NotFound.jsx";
+
...
export const Route = createFileRoute("/playground/$name")({
component: PlaygroundPage,
+ notFoundComponent: NotFound,
+ loader: ({ params }) => {
+ if (!(params.name in modules)) throw notFound();
+ return null;
+ },
- head: ({ params }) => ({
- meta: [{ title: `${kebabCaseToCapitalized(params.name)} — Playground` }],
+ head: ({ params, loaderData }) => ({
+ meta: [
+ {
+ title:
+ loaderData === null
+ ? `${kebabCaseToCapitalized(params.name)} — Playground`
+ : "Not Found — Solid Primitives",
+ },
+ ],
}),
});🤖 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 `@site/src/routes/playground/`$name.tsx~port to tanstack start v1 around lines
25 - 30, Add a loader to the createFileRoute configuration that validates
params.name against the available modules before rendering: inside the Route
created by createFileRoute("/playground/$name") (where PlaygroundPage reads
modules[params().name] and the Dynamic component expects a valid module),
implement a loader that reads params.name, checks modules[params.name] (or the
package registry you use) and if missing returns a 404 Response or throws a
redirect to a not-found route; otherwise return the validated name (or module)
in the loader data so PlaygroundPage consumes a guaranteed-valid value and
avoids passing undefined to Dynamic.
| newValue, | ||
| timeStamp: +new Date(), | ||
| ...(isServer ? {} : { url: location.href }), | ||
| ...(globalThis.window ? { url: location.href } : {}), |
There was a problem hiding this comment.
I still am not sure why we wouldn't just use isServer?
| const output = signal[1](value); | ||
| persist(); | ||
| unchanged = false; | ||
| return output instanceof Promise ? output.then((result) => (persist(), result)) : output; |
There was a problem hiding this comment.
persist() is called synchronously to write the optimistic value, then again in .then() on line 197. There is no catch so if the action rejects, Solid rolls back the in-memory signal, but storage is never re-synced. It permanently holds the pre-rejection optimistic value. On the next page load the signal hydrates with stale data?
| @@ -241,7 +249,7 @@ export const wsSync = (ws: WebSocket, warnOnError: boolean = isDev): Persistence | |||
| key, | |||
| newValue, | |||
| timeStamp: +new Date(), | |||
There was a problem hiding this comment.
Isn't Date.now() more the idiomatic form because it avoids creating an unnecessary date object?
| return [signal[0], signal[1], null] as unknown as PersistedState<S>; | ||
| } | ||
| const storageOptions = (options as unknown as { storageOptions: O }).storageOptions; | ||
| const serialize: (data: T) => string = options.serialize || JSON.stringify.bind(JSON); |
There was a problem hiding this comment.
I was confused by the bind, neither method uses this...so isn't this just allocating a pointless closure? Maybe I missed something? lol
| }); | ||
| } | ||
|
|
||
| const getter = typeof signal[0] === "function" ? signal[0] as () => T : () => snapshot(signal[0] as T); |
There was a problem hiding this comment.
You're going a few of these evaluations on line 140, 166 and 179. Maybe just have const isSignal = ...?
3 participants
Summary by CodeRabbit
New Features
Chores
Refactor
makePersistedAPI with improved type inference and setter behavior