upgrade: list package upgrade for Solid 2.0#936
Conversation
|
|
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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/list/test/index.bench.tsx (1)
8-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset per-iteration state and dispose previous roots inside each bench run.
renderedFor/renderedListpersist across iterations, and previous renders are not disposed untilafterAll. That can resolve later iterations too early and leak roots, making timings unreliable.Suggested patch
bench( "For", () => new Promise(resolve => { + unmountFor(); + renderedFor.clear(); const ItemFor = (props: { number: number }) => { onSettled(() => { renderedFor.add(props.number); if (renderedFor.size === ITEMS) { resolve(); @@ bench( "List", () => new Promise(resolve => { + unmountList(); + renderedList.clear(); const ItemList = (props: { number: number }) => { onSettled(() => { renderedList.add(props.number); if (renderedList.size === ITEMS) { resolve();Also applies to: 17-31, 34-39, 43-58
🤖 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/list/test/index.bench.tsx` around lines 8 - 13, Each bench iteration must reset per-iteration state and dispose previous roots: before starting a new run call the previous unmount functions (unmountFor, unmountList, etc.) to dispose roots and remove appended container nodes, and then clear or recreate the tracking sets (renderedFor, renderedList) and DOM containers (divFor, divList) so state doesn't persist across iterations; update the bench setup where renderedFor/renderedList and unmountFor/unmountList are initialized to ensure you call the prior unmount, remove the container from document.body, and reset the Set (or assign a new Set) at the start of each iteration.
🧹 Nitpick comments (1)
packages/list/README.md (1)
111-111: ⚡ Quick winTighten the children invocation guarantee wording.
The parenthetical on Line 111 (“beyond its previous maximum length”) can over-constrain the rule. Prefer documenting only the invariant:
childrenruns when a new element is created.Proposed doc wording
-- The `children` render function is called **only when a new element is created** (i.e. the array has grown beyond its previous maximum length). +- The `children` render function is called **only when a new element is created**.🤖 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/list/README.md` at line 111, The sentence about the `children` render function is over-constrained by the parenthetical; update the wording to state the invariant concisely: "The `children` render function is called only when a new element is created." Remove the parenthetical "(i.e. the array has grown beyond its previous maximum length)" and replace the line referencing `children` with this tightened sentence so the README documents only the guarantee that `children` runs on creation of a new element.
🤖 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/list/src/index.ts`:
- Around line 206-218: The mapped array is not cleared before returning
fallback, retaining references to disposed item roots; update the early-return
branch (the block using mapped, fallback, fallbackDisposer, runWithOwner,
createRoot and listOwner) to reset/clear mapped (e.g., mapped.length = 0 or
mapped = []) immediately after disposing old item roots and before
creating/returning the fallback so disposed entries are not strongly referenced.
In `@packages/list/test/debug.test.tsx`:
- Around line 7-45: Replace the debug-only console.log checks in the "index
signal updates with one flush" test with real assertions: assert the initial
callbacks and DOM match startingArray ([{v:1,i:0},{v:2,i:1},{v:3,i:2},{v:4,i:3}]
and DOM ["0: 2","1: 4","2: 6","3: 8"]), assert that immediately after
set([1,3,2,4]) but before flush the callbacks/DOM remain unchanged, then call
flush() and assert callbacks/DOM reflect the swapped order
([{v:1,i:0},{v:3,i:1},{v:2,i:2},{v:4,i:3}] and DOM ["0: 2","1: 6","2: 4","3:
8"]), and finally assert a second flush does not change anything; use the
existing createSignal, set, flush, render, List and unmount symbols and the
callbacks array to read values and use your test framework's expect/assert
helpers instead of console.log (or remove the test from CI if you prefer).
---
Outside diff comments:
In `@packages/list/test/index.bench.tsx`:
- Around line 8-13: Each bench iteration must reset per-iteration state and
dispose previous roots: before starting a new run call the previous unmount
functions (unmountFor, unmountList, etc.) to dispose roots and remove appended
container nodes, and then clear or recreate the tracking sets (renderedFor,
renderedList) and DOM containers (divFor, divList) so state doesn't persist
across iterations; update the bench setup where renderedFor/renderedList and
unmountFor/unmountList are initialized to ensure you call the prior unmount,
remove the container from document.body, and reset the Set (or assign a new Set)
at the start of each iteration.
---
Nitpick comments:
In `@packages/list/README.md`:
- Line 111: The sentence about the `children` render function is
over-constrained by the parenthetical; update the wording to state the invariant
concisely: "The `children` render function is called only when a new element is
created." Remove the parenthetical "(i.e. the array has grown beyond its
previous maximum length)" and replace the line referencing `children` with this
tightened sentence so the README documents only the guarantee that `children`
runs on creation of a new element.
🪄 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: 5379a9d3-93da-4e8c-b44d-a3079848ec9f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
packages/list/README.mdpackages/list/dev/index.tsxpackages/list/package.jsonpackages/list/src/index.tspackages/list/stories/list.stories.tsxpackages/list/test/debug.test.tsxpackages/list/test/index.bench.tsxpackages/list/test/index.test.tsxpackages/list/test/server.test.tspackages/list/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/list/dev/index.tsx
maciek50322
left a comment
maciek50322
left a comment
There was a problem hiding this comment.
Looks good.
First I thought of just copying new <For> and just adding some lines.
Can't build this project probably because I'm on windows or missing something
I just went with a raw porting, if you think it could align more closely with Solid 2.0 that way then we should probably consider doing just that? |
2 participants
Adds four Storybook stories for
@solid-primitives/listunderControl Flow/List: index tracking on reorder, in-place value updates, fallback on empty, and directlistArrayusagesrc/index.ts(T→Exclude<T, Function>cast oncreateSignal)src/index.ts— expandedlistArrayandListdocs, per-fieldListItemcomments, and plain-English labels on all six reconciliation passesREADME.mdwith a<For>/<Index>/<List>comparison table, per-primitive prose and code examples, a "when to use" guide, props table, and reactivity guaranteesdev/folder; adds thedocs & demosbadgeSummary by CodeRabbit
New Features
Documentation
Tests