upgrade: map package upgrade for Solid 2.0#937
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 |
🦋 Changeset detectedLatest commit: aee64ef 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 |
![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
🧹 Nitpick comments (1)
packages/map/test/index.test.ts (1)
445-496: ⚡ Quick winAdd a regression case for
clear()when an existing key storesundefined.Please extend this test to assert that
get(existingUndefinedKey)does not re-run onclear(). That locks in the intendedget()value-change semantics and protects theclear()edge case.🤖 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/map/test/index.test.ts` around lines 445 - 496, Add a regression case that seeds the ReactiveMap with a key whose value is explicitly undefined, create an effect that depends on map.get(existingUndefinedKey) (use the same pattern as existingValue/nonexistingValue), call flush and assert the effect ran once with undefined, then call map.clear() and flush and assert that this get-effect did NOT re-run (still called once); reference the ReactiveMap instance, map.get(key) effect, and map.clear() to locate where to add the new effect and the corresponding expect assertions.
🤖 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/map/src/index.ts`:
- Around line 121-125: clear() is notifying per-key value subscribers for every
existing key, causing get(key) listeners to run even when the stored value was
already undefined; make clear() mirror delete() semantics by only calling
this.#valueTriggers.dirty(key) for keys whose stored value is not undefined.
Locate the clear() implementation (the loop using for (const key of
super.keys())), and change the logic to check the current value via
this.get(key) (or equivalent) and only call this.#valueTriggers.dirty(key) when
that value !== undefined; still call this.#keyTriggers.dirty(key) for every
existing key as before.
In `@packages/map/stories/map.stories.tsx`:
- Around line 210-222: The clickable row currently implemented as a <div> (the
block using checked.get(item), checked.set(item, !checked.get(item)), colors and
radii) must be changed to a semantic interactive element: replace the <div> with
a <button type="button">, move the inline styles onto the button (preserving
background and border calculations that use checked.get(item)), keep the onClick
handler (checked.set(...)) and also add keyboard/ARIA semantics such as
aria-pressed={checked.get(item)} and remove any default button styles (e.g.,
reset border/background via style) to preserve visuals; this ensures keyboard
users can toggle rows for the element that references checked, item, colors and
radii.
---
Nitpick comments:
In `@packages/map/test/index.test.ts`:
- Around line 445-496: Add a regression case that seeds the ReactiveMap with a
key whose value is explicitly undefined, create an effect that depends on
map.get(existingUndefinedKey) (use the same pattern as
existingValue/nonexistingValue), call flush and assert the effect ran once with
undefined, then call map.clear() and flush and assert that this get-effect did
NOT re-run (still called once); reference the ReactiveMap instance, map.get(key)
effect, and map.clear() to locate where to add the new effect and the
corresponding expect assertions.
🪄 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: 1536d47f-7e32-420d-96e0-95def3ed486d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.changeset/map-solid2-migration.mdpackages/map/CHANGELOG.mdpackages/map/README.mdpackages/map/dev/index.tsxpackages/map/package.jsonpackages/map/src/index.tspackages/map/stories/map.stories.tsxpackages/map/test/index.test.tspackages/map/test/server.test.ts
💤 Files with no reviewable changes (1)
- packages/map/dev/index.tsx
| this.#valueTriggers.dirty(key); | ||
| } | ||
| }); | ||
| if (hadNoKey || hasChanged) { |
There was a problem hiding this comment.
The surrounding if is actually meaningless.
if (hadNoKey || hasChanged) {
if (hadNoKey) { ... }
if (hasChanged) { ... }
}
// has the same effect as:
if (hadNoKey) { ... }
if (hasChanged) { ... }| if (hadNoKey) this.#keyTriggers.dirty(key); | ||
| if (hasChanged) this.#valueTriggers.dirty(key); | ||
| }); | ||
| if (hadNoKey || hasChanged) { |
There was a problem hiding this comment.
Again, the surrounding if is superfluous.
2 participants
Summary
@solid-primitives/mapto Solid.js^2.0.0-beta.14Solid 2.0 migration
Implementation (
src/index.ts)batch()wrappers fromset,delete, andclear— Solid 2.0 auto-batches all signal writes, making them redundantTests (
test/index.test.ts,test/server.test.ts)createComputed(removed in 2.0) withcreateEffect(compute, apply)split form throughoutcreateEffectcalls to the required split formflush()after each write before assertionstest/server.test.ts— 2 SSR safety tests confirmingReactiveMapandReactiveWeakMapoperate correctly on the server (trigger tracking is a no-op viaisServerin the underlyingTriggerCache)Package
solid-js@^2.0.0-beta.14,@solidjs/web@^2.0.0-beta.14devscript (replaced by Storybook)Summary by CodeRabbit
New Features
Documentation
Chores