upgrade: set package upgrade for Solid 2.0#911
Open
davedbase wants to merge 2 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: b275495 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 |
|
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/set/README.md (2)
100-101: ⚡ Quick winClarify or remove test-specific terminology.
The comment "after flush:" references Solid's testing utility and may confuse users unfamiliar with that API. Consider either:
- Removing the comment entirely (the example works without it)
- Rephrasing to user-friendly language:
// after reactivity updates complete:- Or briefly explaining:
// after reactivity updates (use flush() in tests):📝 Suggested rewording
a.add(5); -// after flush: +// after reactivity updates complete: u(); // => Set {1, 2, 3, 4, 5}🤖 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/set/README.md` around lines 100 - 101, The inline comment "after flush:" in packages/set/README.md is test-specific and may confuse users; update the comment next to the example invoking u() to either remove it or replace it with a user-friendly phrase such as "// after reactivity updates complete:" or "// after reactivity updates (use flush() in tests):" so the example for u(); // => Set {1, 2, 3, 4, 5} is clear to non-test readers.
43-50: ⚡ Quick winUpdate the createEffect concern: the 2-argument examples are valid in Solid.js 2.0
Solid.js 2.0 supports the split-phasecreateEffect(compute, apply)form, so the README examples at lines 43-50 / 69-72 don’t need to be rewritten to the single-callback style.
- Consider changing “after flush:” (line 100) if that wording is intended to be test/internal terminology rather than user-facing docs.
- Consider minor wording tweaks (e.g., repeated “Elements …” section starts) to reduce stylistic repetition.
🤖 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/set/README.md` around lines 43 - 50, The README currently suggests rewriting the two-argument createEffect examples, but Solid.js 2.0 supports the split-phase form; keep the examples using createEffect(() => [...set], values => ...) and createEffect(() => set.has(2), exists => ...) as valid. Also update the phrase "after flush:" (if intended for users) to a clearer user-facing term or mark it as internal/test-only, and collapse or reword duplicate section headings like the repeated "Elements …" to reduce repetition. Locate references to createEffect, the set examples, and the "after flush:" heading in the README to make these edits.
🤖 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/set/test/index.test.ts`:
- Around line 408-423: The test currently calls fn() directly so it never
subscribes to reactivity; change it to run inside a reactive observer (e.g.,
inside createEffect/createRoot) so the accessor returned by intersection(a, b)
is tracked. Specifically, replace the manual fn() invocation with wiring like
createEffect(() => fn([...intersection(a, b)()])), ensure the first invocation
occurs as part of effect setup, then call b.add(99) and flush(), and assert
expect(fn).toHaveBeenCalledTimes(1) on the observer function (not an untracked
call); keep createRoot, ReactiveSet, intersection, flush, and dispose usage
intact.
---
Nitpick comments:
In `@packages/set/README.md`:
- Around line 100-101: The inline comment "after flush:" in
packages/set/README.md is test-specific and may confuse users; update the
comment next to the example invoking u() to either remove it or replace it with
a user-friendly phrase such as "// after reactivity updates complete:" or "//
after reactivity updates (use flush() in tests):" so the example for u(); // =>
Set {1, 2, 3, 4, 5} is clear to non-test readers.
- Around line 43-50: The README currently suggests rewriting the two-argument
createEffect examples, but Solid.js 2.0 supports the split-phase form; keep the
examples using createEffect(() => [...set], values => ...) and createEffect(()
=> set.has(2), exists => ...) as valid. Also update the phrase "after flush:"
(if intended for users) to a clearer user-facing term or mark it as
internal/test-only, and collapse or reword duplicate section headings like the
repeated "Elements …" to reduce repetition. Locate references to createEffect,
the set examples, and the "after flush:" heading in the README to make these
edits.
🪄 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: f5f9ede7-7a31-4d91-8b06-d1c794f7636d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.changeset/set-solid2-migration.mdpackages/set/README.mdpackages/set/package.jsonpackages/set/src/index.tspackages/set/test/index.test.tspackages/set/test/server.test.ts
Comment on lines
+408
to
+423
| it("does not re-derive when an unshared element is added to b", () => | ||
| createRoot(dispose => { | ||
| const a = new ReactiveSet([1, 2]); | ||
| const b = new ReactiveSet([2]); | ||
| const fn = vi.fn(() => [...intersection(a, b)()]); | ||
|
|
||
| // call once to get initial value and set up tracking | ||
| fn(); | ||
|
|
||
| b.add(99); // 99 is not in a — intersection unchanged | ||
| flush(); | ||
|
|
||
| // fn was not called reactively; memo didn't invalidate | ||
| expect(fn).toHaveBeenCalledTimes(1); | ||
|
|
||
| dispose(); |
There was a problem hiding this comment.
Non-rederive assertion is currently non-reactive and can false-pass.
On Line 412 and Line 415, fn() is called manually and never subscribed in an effect, so expect(fn).toHaveBeenCalledTimes(1) will pass regardless of whether intersection(a, b) invalidates. Please wire the accessor into a reactive observer and assert observer call count instead.
Suggested fix
it("does not re-derive when an unshared element is added to b", () =>
createRoot(dispose => {
const a = new ReactiveSet([1, 2]);
const b = new ReactiveSet([2]);
- const fn = vi.fn(() => [...intersection(a, b)()]);
+ const i = intersection(a, b);
+ const fn = vi.fn();
+ createEffect(() => {
+ fn([...i()]);
+ });
- // call once to get initial value and set up tracking
- fn();
+ flush();
+ expect(fn).toHaveBeenCalledTimes(1);
b.add(99); // 99 is not in a — intersection unchanged
flush();
- // fn was not called reactively; memo didn't invalidate
+ // no re-derivation expected
expect(fn).toHaveBeenCalledTimes(1);
dispose();
}));📝 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.
Suggested change
| it("does not re-derive when an unshared element is added to b", () => | |
| createRoot(dispose => { | |
| const a = new ReactiveSet([1, 2]); | |
| const b = new ReactiveSet([2]); | |
| const fn = vi.fn(() => [...intersection(a, b)()]); | |
| // call once to get initial value and set up tracking | |
| fn(); | |
| b.add(99); // 99 is not in a — intersection unchanged | |
| flush(); | |
| // fn was not called reactively; memo didn't invalidate | |
| expect(fn).toHaveBeenCalledTimes(1); | |
| dispose(); | |
| it("does not re-derive when an unshared element is added to b", () => | |
| createRoot(dispose => { | |
| const a = new ReactiveSet([1, 2]); | |
| const b = new ReactiveSet([2]); | |
| const i = intersection(a, b); | |
| const fn = vi.fn(); | |
| createEffect(() => { | |
| fn([...i()]); | |
| }); | |
| flush(); | |
| expect(fn).toHaveBeenCalledTimes(1); | |
| b.add(99); // 99 is not in a — intersection unchanged | |
| flush(); | |
| // no re-derivation expected | |
| expect(fn).toHaveBeenCalledTimes(1); | |
| dispose(); | |
| })); |
🤖 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/set/test/index.test.ts` around lines 408 - 423, The test currently
calls fn() directly so it never subscribes to reactivity; change it to run
inside a reactive observer (e.g., inside createEffect/createRoot) so the
accessor returned by intersection(a, b) is tracked. Specifically, replace the
manual fn() invocation with wiring like createEffect(() =>
fn([...intersection(a, b)()])), ensure the first invocation occurs as part of
effect setup, then call b.add(99) and flush(), and assert
expect(fn).toHaveBeenCalledTimes(1) on the observer function (not an untracked
call); keep createRoot, ReactiveSet, intersection, flush, and dispose usage
intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upgrades
@solid-primitives/setto Solid.js v2.0 (beta.13) and ships five new exports.Migration
batch()wrappers fromReactiveSet.add(),delete(), andclear()— Solid 2.0 auto-batches all signal writes, so the sequentialdirty()calls are already grouped.createEffect(compute, apply)form withflush()calls between mutations and assertions, replacing the removedcreateComputed.test/server.test.tsverifying both classes behave as plain non-reactive data structures on the server.solid-js@^2.0.0-beta.13and@solidjs/web@^2.0.0-beta.13.New exports
union(a, b)a,b, or bothintersection(a, b)aandbdifference(a, b)anot inbsymmetricDifference(a, b)readonlySet(set)ReactiveSettoReadonlySetAll four algebra functions return
Accessor<ReadonlySet<T>>backed bycreateMemo.intersectionanddifferenceuse per-key tracking onb(b.has(v)inside the memo) rather than subscribing tob's$KEYSsignal — meaning adding an element tobthat has no overlap withawill not invalidate the memo.Breaking changes
Requires Solid.js v2.0. No changes to the
ReactiveSetorReactiveWeakSetpublic API.Summary by CodeRabbit
New Features
union,intersection,difference, andsymmetricDifferencefor reactive set computations.readonlySethelper for type-safe read-only views of reactive sets.Documentation
Chores