WIP: feat(solid-form): support Solid 2 beta 6#2122
WIP: feat(solid-form): support Solid 2 beta 6#2122brenelz wants to merge 2 commits intoTanStack:mainfrom
Conversation
Update the package runtime, tests, and Vite test setup for the Solid 2 beta API surface so solid-form can build and test against beta 6.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR updates the solid-form package to SolidJS 2.0.0-beta.6, refactoring lifecycle management from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
View your CI Pipeline Execution ↗ for commit 9216ee2 ☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/solid-form/tests/createField.test.tsx`:
- Line 4: The test imports Repeat from 'solid-js' which doesn't exist in Solid
2.0; remove the Repeat import and replace any usage of Repeat with For
keyed={false} from 'solid-js' (keep Show import as-is), adapting the render
callback to the For signature (item, i) => ... where item and i are accessors;
if you actually need an index-only Repeat component, import it from
`@solid-primitives/range` instead.
In `@packages/solid-form/tests/testing-library.ts`:
- Around line 23-25: The container selection logic currently can remove
caller-provided containers and fails to clean up auto-created containers under a
custom baseElement, and returning raw dispose prevents deregistering on manual
unmount; fix by introducing an ownedContainer boolean (set true when you create
the div for container) next to baseElement/container logic, wrap the returned
dispose in a custom unmount function that (1) calls the original dispose, (2) if
ownedContainer is true removes the container from the DOM, and (3) removes the
entry from your internal registry so cleanup() only iterates and removes
containers that were created (owned) by this module; update cleanup() to check
ownedContainer/registry flags before removing nodes and ensure all places return
the custom unmount instead of the raw dispose.
In `@packages/solid-form/vite.config.ts`:
- Line 3: ESLint flagged the import order: move the "fileURLToPath" import from
node:url so it appears before the vitest/config import; update the top of
vite.config.ts to import { fileURLToPath } from 'node:url' prior to any import
from 'vitest/config' (so adjust the sequence around the existing import lines in
this file).
🪄 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
Run ID: 6909c33b-d852-4dc9-a4b4-d20422c73d6a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
packages/solid-form/package.jsonpackages/solid-form/src/createField.tsxpackages/solid-form/src/createFieldGroup.tsxpackages/solid-form/src/createForm.tsxpackages/solid-form/src/createFormHook.tsxpackages/solid-form/tests/createField.test.tsxpackages/solid-form/tests/createForm.test.tsxpackages/solid-form/tests/createFormHook.test.tsxpackages/solid-form/tests/testing-library.tspackages/solid-form/vite.config.ts
| import { render, waitFor } from '@solidjs/testing-library' | ||
| import { userEvent } from '@testing-library/user-event' | ||
| import { Index, Show } from 'solid-js' | ||
| import { Repeat, Show } from 'solid-js' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SolidJS 2.0 Repeat component API signature
💡 Result:
SolidJS 2.0 does not have a built-in "Repeat" component in core. Core control flow components are , , /, (replaces Suspense), and (replaces ErrorBoundary). In SolidJS 2.0 beta: - from 1.x is replaced by - children now receive accessors: (item, i) => ... where item and i are called. A component exists in the third-party @solid-primitives/range package (updated Feb 2026), with API: function Repeat<T>(props: { times: number; fallback?: T; children: ((index: number) => T) | T; }): Accessor<T[]>; Example: <Repeat times={10}> <div></div> </Repeat> or with render prop: <Repeat times={10}> {index => <div>{index}</div>} </Repeat> No evidence of core Repeat in 2.0 docs/RFCs/release notes. Official 2.0 docs at docs.solidjs.com cover 1.x; 2.0 migration in GitHub next branch.
Citations:
- 1: https://github.com/solidjs/solid/releases/tag/v2.0.0-beta.0
- 2: https://github.com/solidjs/solid/blob/next/documentation/solid-2.0/MIGRATION.md
- 3: https://docs.bswen.com/blog/2026-03-12-solidjs-async-handling
- 4: https://primitives.solidjs.community/package/range/
- 5: https://registry.npmjs.org/%40solid-primitives%2Frange
Repeat is not exported from solid-js 2.0 — use <For keyed={false}> instead.
Repeat does not exist in SolidJS 2.0 core. The import at line 4 will fail. The correct migration from Index is <For keyed={false}>, which receives the signature (item, i) => ... where both are accessor functions. If a Repeat component is needed for index-only iteration, it exists as a third-party component in @solid-primitives/range (not in core solid-js).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/solid-form/tests/createField.test.tsx` at line 4, The test imports
Repeat from 'solid-js' which doesn't exist in Solid 2.0; remove the Repeat
import and replace any usage of Repeat with For keyed={false} from 'solid-js'
(keep Show import as-is), adapting the render callback to the For signature
(item, i) => ... where item and i are accessors; if you actually need an
index-only Repeat component, import it from `@solid-primitives/range` instead.
| const baseElement = options.baseElement ?? document.body | ||
| const container = | ||
| options.container ?? baseElement.appendChild(document.createElement('div')) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/solid-form/tests/testing-library.tsRepository: TanStack/form
Length of output: 2198
Track container ownership and deregister on unmount to prevent cleanup bugs.
Current implementation has three reliability problems:
- Line 60 can remove caller-provided containers. The parentNode check doesn't distinguish between auto-created and caller-managed containers.
- Auto-created containers under custom
baseElementare not cleaned up (parentNode won't equal document.body). - Line 51 returns raw
dispose, so manualunmount()calls don't remove the registry entry, risking stale references and double disposal whencleanup()runs.
Add ownedContainer boolean to track ownership, wrap cleanup logic in a custom unmount function, and update cleanup() to only remove containers this module created:
🔧 Proposed fix
type RenderRef = {
container: HTMLElement
dispose: () => void
+ ownedContainer: boolean
}
@@
export function render(
@@
) {
const baseElement = options.baseElement ?? document.body
- const container =
- options.container ?? baseElement.appendChild(document.createElement('div'))
+ const ownedContainer = !options.container
+ const container =
+ options.container ?? baseElement.appendChild(document.createElement('div'))
@@
- mountedContainers.add({ container, dispose })
+ const ref: RenderRef = { container, dispose, ownedContainer }
+ mountedContainers.add(ref)
@@
+ const unmount = () => {
+ ref.dispose()
+ if (ref.ownedContainer && ref.container.parentNode) {
+ ref.container.parentNode.removeChild(ref.container)
+ }
+ mountedContainers.delete(ref)
+ }
+
return {
@@
- unmount: dispose,
+ unmount,
...queryHelpers,
}
}
@@
export function cleanup() {
for (const ref of Array.from(mountedContainers)) {
ref.dispose()
- if (ref.container.parentNode === document.body) {
- document.body.removeChild(ref.container)
+ if (ref.ownedContainer && ref.container.parentNode) {
+ ref.container.parentNode.removeChild(ref.container)
}
mountedContainers.delete(ref)
}
}📝 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 baseElement = options.baseElement ?? document.body | |
| const container = | |
| options.container ?? baseElement.appendChild(document.createElement('div')) | |
| type RenderRef = { | |
| container: HTMLElement | |
| dispose: () => void | |
| ownedContainer: boolean | |
| } | |
| export function render( | |
| Component: ParentComponent, | |
| options: RenderOptions = {}, | |
| ) { | |
| const baseElement = options.baseElement ?? document.body | |
| const ownedContainer = !options.container | |
| const container = | |
| options.container ?? baseElement.appendChild(document.createElement('div')) | |
| // ... component mounting code ... | |
| const ref: RenderRef = { container, dispose, ownedContainer } | |
| mountedContainers.add(ref) | |
| const unmount = () => { | |
| ref.dispose() | |
| if (ref.ownedContainer && ref.container.parentNode) { | |
| ref.container.parentNode.removeChild(ref.container) | |
| } | |
| mountedContainers.delete(ref) | |
| } | |
| return { | |
| container, | |
| unmount, | |
| ...queryHelpers, | |
| } | |
| } | |
| export function cleanup() { | |
| for (const ref of Array.from(mountedContainers)) { | |
| ref.dispose() | |
| if (ref.ownedContainer && ref.container.parentNode) { | |
| ref.container.parentNode.removeChild(ref.container) | |
| } | |
| mountedContainers.delete(ref) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/solid-form/tests/testing-library.ts` around lines 23 - 25, The
container selection logic currently can remove caller-provided containers and
fails to clean up auto-created containers under a custom baseElement, and
returning raw dispose prevents deregistering on manual unmount; fix by
introducing an ownedContainer boolean (set true when you create the div for
container) next to baseElement/container logic, wrap the returned dispose in a
custom unmount function that (1) calls the original dispose, (2) if
ownedContainer is true removes the container from the DOM, and (3) removes the
entry from your internal registry so cleanup() only iterates and removes
containers that were created (owned) by this module; update cleanup() to check
ownedContainer/registry flags before removing nodes and ensure all places return
the custom unmount instead of the raw dispose.
| @@ -1,10 +1,20 @@ | |||
| import { defineConfig, mergeConfig } from 'vitest/config' | |||
| import { tanstackViteConfig } from '@tanstack/vite-config' | |||
| import { fileURLToPath } from 'node:url' | |||
There was a problem hiding this comment.
Import order issue flagged by ESLint.
The node:url import should be placed before vitest/config to follow the project's import ordering conventions.
🔧 Suggested fix
+import { fileURLToPath } from 'node:url'
import { defineConfig, mergeConfig } from 'vitest/config'
import { tanstackViteConfig } from '@tanstack/vite-config'
-import { fileURLToPath } from 'node:url'
import solid from 'vite-plugin-solid'🧰 Tools
🪛 ESLint
[error] 3-3: node:url import should occur before import of vitest/config
(import/order)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/solid-form/vite.config.ts` at line 3, ESLint flagged the import
order: move the "fileURLToPath" import from node:url so it appears before the
vitest/config import; update the top of vite.config.ts to import { fileURLToPath
} from 'node:url' prior to any import from 'vitest/config' (so adjust the
sequence around the existing import lines in this file).
Update solid-form packaging, supporting examples, and Solid package manifests so the Solid 2 beta toolchain resolves consistently across the workspace.
1 participant
Update the package runtime, tests, and Vite test setup for the Solid 2 beta API surface so solid-form can build and test against beta 6.
Summary by CodeRabbit
Dependencies
Improvements