new: url primitive for Solid 2.0#978
Conversation
davedbase
added
the
Primitive Proposal
🦋 Changeset detectedLatest commit: e310c4f 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:
📝 WalkthroughWalkthroughAdds a new ChangesNew
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Component
participant createLocationState
participant History
participant Window
Component->>createLocationState: push/replace/navigate(fields)
createLocationState->>createLocationState: build URL from location.href
createLocationState->>History: pushState/replaceState (or set location.href)
History-->>Window: popstate/hashchange
Window-->>createLocationState: notify listeners
createLocationState-->>Component: update reactive LocationState store
sequenceDiagram
participant Component
participant ReactiveURL
participant ReactiveSearchParams
Component->>ReactiveURL: set search = "?a=1"
ReactiveURL->>ReactiveURL: syncSearchParamsFromSearch()
ReactiveURL->>ReactiveSearchParams: apply diffed entries
Component->>ReactiveSearchParams: set("a", "2")
ReactiveSearchParams-->>ReactiveURL: onChange callback
ReactiveURL->>ReactiveURL: update .search from searchParams.toString()
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 4
🧹 Nitpick comments (5)
packages/url/test/location.test.ts (1)
59-65: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider a regression test for the
(key, value)overload withorigin.Only the record form (
push({ origin: ..., pathname: ... })) is tested for the origin-immutability guarantee (Lines 75-83). Given the asymmetry noted inlocation.ts's setter (record branch guardsorigin, key/value branch doesn't), a test likepush("origin", "http://evil.com")would have caught that gap.Also applies to: 75-83
🤖 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/url/test/location.test.ts` around lines 59 - 65, Add a regression test for the createLocationState push setter’s key/value overload to cover origin immutability, since only the record overload currently verifies that behavior. In location.test.ts, extend the existing push-related tests around createLocationState and push so that calling push("origin", "http://evil.com") is asserted to leave the state.origin unchanged, matching the protection already tested in the record-form branch of the setter.packages/url/test/searchParams.test.ts (1)
27-91: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing regression test for
sort().None of the
ReactiveSearchParamstests exercise.sort(). As flagged insearchParams.ts(lines 196-199),sort()skips theonChangenotification that other mutators call, which — once fixed — should be locked in with a test (e.g. assertingtoString()reflects the sorted order and that aWHOLE-tracking effect reruns).🤖 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/url/test/searchParams.test.ts` around lines 27 - 91, Add a regression test in ReactiveSearchParams coverage for sort() to lock in the missing onChange behavior. Use ReactiveSearchParams plus createEffect/flush in the test suite to assert that calling sort() updates toString() into sorted order and reruns a WHOLE-tracking effect, mirroring how other mutators are tested.packages/url/test/url.test.ts (1)
78-130: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a
sort()sync test here too.Given
sort()is a mutator that should re-serialize ontourl.searchon a nativeURL(per MDN, anysearchParamsupdate — including sort — re-serializes onto.search), thissearchParamsdescribe block would be a natural place for a regression test once thesort()/onChangebug insearchParams.tsis fixed.🤖 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/url/test/url.test.ts` around lines 78 - 130, Add a regression test in the searchParams describe block for the missing sort() sync behavior. Use createURL, url.searchParams.sort(), and flush() to verify that calling sort() re-serializes back onto url.search and keeps searchParams and url.search in sync, similar to the existing mutator tests in url.test.ts.packages/url/src/searchParams.ts (2)
133-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClass JSDoc gets detached by the
WHOLEsymbol declaration.The doc comment intended for
ReactiveSearchParams(lines 133-148) is immediately followed byWHOLE's own doc comment and declaration (149-150), then a blank line beforeexport class(152). Editors associate a JSDoc with the next declaration only when adjacent — as written, the class's real doc-comment won't show up in IDE tooltips/hover forReactiveSearchParams.♻️ Suggested reordering
-/** - * A reactive version of the `URLSearchParams` class. Every read method is granular — it causes - * an update only when the value it read has actually changed. - * ... - */ -/** Tracking key standing in for "any part of the collection" — reads that iterate the whole thing (`toString`, `keys`, `entries`, `values`, `forEach`) depend on it, and every mutation dirties it. */ -const WHOLE = Symbol("whole"); - -export class ReactiveSearchParams extends URLSearchParams { +/** Tracking key standing in for "any part of the collection" — reads that iterate the whole thing (`toString`, `keys`, `entries`, `values`, `forEach`) depend on it, and every mutation dirties it. */ +const WHOLE = Symbol("whole"); + +/** + * A reactive version of the `URLSearchParams` class. Every read method is granular — it causes + * an update only when the value it read has actually changed. + * ... + */ +export class ReactiveSearchParams extends URLSearchParams {🤖 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/url/src/searchParams.ts` around lines 133 - 151, The `ReactiveSearchParams` JSDoc is being separated from the class by the `WHOLE` symbol comment/declaration, so move the `WHOLE` tracking-key comment and declaration to stay with `WHOLE` and keep the `ReactiveSearchParams` doc block immediately adjacent to the `export class ReactiveSearchParams` declaration. Ensure the class’s JSDoc directly precedes `ReactiveSearchParams`, and place the `WHOLE` documentation right above `const WHOLE = Symbol("whole")` so IDE hover/tooltips attach correctly.
185-189: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
delete/hasoverrides drop the spec's optionalvalueparameter.Per MDN,
delete()andhas()on nativeURLSearchParamsboth accept an optional secondvalueargument to match a specific name-value pair rather than all entries with that name: delete(name) delete(name, value) name · The name of the parameters to be deleted. value Optional · The value that parameters must match, along with the given name, to be deleted. The overrides here only acceptname: string, so callingsearchParams.delete("foo", "bar")or.has("foo", "bar")on aReactiveSearchParamssilently ignores the value filter — a behavioral divergence from the class's stated goal of behaving "likeURLSearchParams". Worth confirming whether the target runtime/browser matrix for this package needs to support this and, if so, extending the overrides to accept and forward the optional value.Also applies to: 210-213
🤖 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/url/src/searchParams.ts` around lines 185 - 189, The ReactiveSearchParams overrides for delete and has are missing the native URLSearchParams optional value argument, so calls that target a specific name-value pair behave incorrectly. Update the delete and has methods in ReactiveSearchParams to accept the optional second value parameter and forward it to the underlying super implementation, preserving the existing notify behavior after mutations.
🤖 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/url/src/location.ts`:
- Around line 149-168: The single-key path in the setter overload allows
assigning to `origin` without a runtime guard, unlike the record branch that
already skips it. Update the `setter` logic in `location.ts` so the `typeof a
=== "string"` branch also rejects or ignores `"origin"` before writing to
`url[a]`, matching the existing protection in the `(key, value)` record handling
and preventing the strict-mode throw from `URL.prototype.origin`.
- Line 43: The location fallback and shared location state are currently
process-wide singletons, which can leak state across concurrent SSR requests.
Update the location handling in location.ts so `setLocationFallback` is not
stored in a module-level variable shared by the whole Node process, and replace
`useSharedLocationState`’s `createSingletonRoot`-based behavior with
`createHydratableSingletonRoot` or restrict it from server rendering. Keep the
fix centered around the `LocationState`, `setLocationFallback`, and
`useSharedLocationState` symbols so the state is request-safe during SSR.
In `@packages/url/src/searchParams.ts`:
- Around line 196-199: The sort() override in ReactiveURLSearchParams currently
only marks the whole object dirty and bypasses the change notification path,
which leaves ReactiveURL.search and href out of sync after
url.searchParams.sort(). Update sort() to use the existing `#notify`() flow
instead of calling `#dirty`(WHOLE) directly, so the onChange callback runs and the
parent URL state stays synchronized. Add a regression test around
ReactiveURLSearchParams.sort() to verify the URL search string and href update
correctly after sorting.
- Around line 161-172: The ReactiveSearchParams constructor currently copies
from an existing instance by calling init.toString() directly, which creates an
unintended tracked read on the source object's WHOLE trigger. Wrap the copy path
in untrack so constructing from another ReactiveSearchParams performs a
one-time, dependency-free read, and apply this fix in the constructor of
ReactiveSearchParams (with the needed solid-js untrack import).
---
Nitpick comments:
In `@packages/url/src/searchParams.ts`:
- Around line 133-151: The `ReactiveSearchParams` JSDoc is being separated from
the class by the `WHOLE` symbol comment/declaration, so move the `WHOLE`
tracking-key comment and declaration to stay with `WHOLE` and keep the
`ReactiveSearchParams` doc block immediately adjacent to the `export class
ReactiveSearchParams` declaration. Ensure the class’s JSDoc directly precedes
`ReactiveSearchParams`, and place the `WHOLE` documentation right above `const
WHOLE = Symbol("whole")` so IDE hover/tooltips attach correctly.
- Around line 185-189: The ReactiveSearchParams overrides for delete and has are
missing the native URLSearchParams optional value argument, so calls that target
a specific name-value pair behave incorrectly. Update the delete and has methods
in ReactiveSearchParams to accept the optional second value parameter and
forward it to the underlying super implementation, preserving the existing
notify behavior after mutations.
In `@packages/url/test/location.test.ts`:
- Around line 59-65: Add a regression test for the createLocationState push
setter’s key/value overload to cover origin immutability, since only the record
overload currently verifies that behavior. In location.test.ts, extend the
existing push-related tests around createLocationState and push so that calling
push("origin", "http://evil.com") is asserted to leave the state.origin
unchanged, matching the protection already tested in the record-form branch of
the setter.
In `@packages/url/test/searchParams.test.ts`:
- Around line 27-91: Add a regression test in ReactiveSearchParams coverage for
sort() to lock in the missing onChange behavior. Use ReactiveSearchParams plus
createEffect/flush in the test suite to assert that calling sort() updates
toString() into sorted order and reruns a WHOLE-tracking effect, mirroring how
other mutators are tested.
In `@packages/url/test/url.test.ts`:
- Around line 78-130: Add a regression test in the searchParams describe block
for the missing sort() sync behavior. Use createURL, url.searchParams.sort(),
and flush() to verify that calling sort() re-serializes back onto url.search and
keeps searchParams and url.search in sync, similar to the existing mutator tests
in url.test.ts.
🪄 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: 8bf75d35-c961-4858-93bf-2bbc27752024
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.changeset/url-initial.mdREADME.mdpackages/analytics/package.jsonpackages/url/LICENSEpackages/url/README.mdpackages/url/package.jsonpackages/url/src/common.tspackages/url/src/index.tspackages/url/src/location.tspackages/url/src/searchParams.tspackages/url/src/url.tspackages/url/stories/location.stories.tsxpackages/url/stories/locationSearchParams.stories.tsxpackages/url/stories/searchParams.stories.tsxpackages/url/stories/url.stories.tsxpackages/url/test/location.test.tspackages/url/test/searchParams.test.tspackages/url/test/server.test.tspackages/url/test/url.test.tspackages/url/tsconfig.json
…eates an implicit dependency
2 participants
Attempts to provide a solution for #427 and #770
Adds
@solid-primitives/url, adapted from the design proposed in the unmerged PR #77, rewritten from scratch for Solid 2.0:createLocationState/useSharedLocationState/updateLocation/setLocationFallback— reactivewindow.location, withpush/replace/navigatesetterscreateURL/ReactiveURL/createURLRecord— a granularly reactiveURL-like object and storecreateSearchParams/ReactiveSearchParams/createLocationSearchParams/useSharedLocationSearchParams/getSearchParamsRecord— a granularly reactiveURLSearchParams-like object, standalone or bound to the browser's location@solid-primitives/static-storefor the shallow reactive records,@solid-primitives/trigger'sTriggerCacheforReactiveSearchParams' granular per-key tracking, acreateStore(fn, seed)projection for the dynamic-keyedSearchParamsRecord, and a guarded imperative two-way sync between.searchand.searchParams(replacing the removedcreateComputed/on)this.searchread duringReactiveSearchParams's lazy construction, and a stale post-write read caused by Solid 2's write-batching once.searchis bound reactively elsewhereBrowser APIs/URL) and a full test suite (33 tests: 28 client + 5 SSR)Summary by CodeRabbit
New Features
Documentation
Tests