fix: make `SvelteURLSearchParams` notifications accurate (#18425)

`SvelteURLSearchParams` has two notification bugs that surface through
`SvelteURL`.

The first is over-notification. Setting `url.href` always rebuilds the
params and bumps their version signal, even when the search string
didn't change at all. Effects that read `size` (or any params method)
re-run on every unrelated `href` write, which is #17218, where it caused
infinite effect loops in a router. The fix is a guard in the internal
replace path that bails out when the incoming params serialize
identically to the current ones.

The second is under-notification, and it's sneakier. Every read method
tracks the version signal, `get`, `getAll`, `has`, `keys`, `values`,
`entries`, `toString`, `size`, the iterator... except `forEach`, which
was never overridden. The inherited platform method reads the internal
list directly and subscribes to nothing, so a template that renders
params via `forEach` never updates:

```js
$effect(() => {
	url.searchParams.forEach((value, key) => entries.push(`${key}=${value}`)); // never re-runs
});
```

We found this one in the wild while making `page.url` a `SvelteURL` in
sveltejs/kit#16031. Kit's own test suite renders query params with
`forEach`, and the coarse object-identity signal that previously masked
the gap went away, leaving stale UI after client-side navigations. The
fix is the same one-line tracking pattern every other read method
already uses.

Fixes #17218

---

- [x] It's really useful if your PR references an issue where it is
discussed ahead of time. In many cases, features are absent for a
reason. For large changes, please create an RFC:
https://github.com/sveltejs/rfcs
- [x] Prefix your PR title with `feat:`, `fix:`, `chore:`, or `docs:`.
- [x] This message body should clearly illustrate what problems it
solves.
- [x] Ideally, include a test that fails without this PR but passes with
it.
- [x] If this PR changes code within `packages/svelte/src`, add a
changeset (`npx changeset`).

### Tests and linting

- [x] Run the tests with `pnpm test` and lint the project with `pnpm
lint`
pull/18448/head
Nic Polumeyv 3 months ago committed by GitHub
parent 8ea3ee8c62
commit 0510174bc0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: don't notify `searchParams` subscribers when the URL changes without affecting the search string

@ -54,6 +54,11 @@ export class SvelteURLSearchParams extends URLSearchParams {
*/
[REPLACE](params) {
if (this.#updating) return;
// the URL may have changed in a way that leaves the search string untouched —
// don't rebuild the params or notify readers if nothing changed
if (params.toString() === super.toString()) return;
this.#updating = true;
for (const key of [...super.keys()]) {
@ -126,6 +131,16 @@ export class SvelteURLSearchParams extends URLSearchParams {
return super.keys();
}
/**
* @param {(value: string, key: string, parent: URLSearchParams) => void} callback
* @param {any} [this_arg]
* @returns {void}
*/
forEach(callback, this_arg) {
get(this.#version);
super.forEach(callback, this_arg);
}
/**
* @param {string} name
* @param {string} value

@ -244,3 +244,24 @@ test('URLSearchParams.toString', () => {
test('SvelteURLSearchParams instanceof URLSearchParams', () => {
assert.ok(new SvelteURLSearchParams() instanceof URLSearchParams);
});
test('params.forEach is reactive', () => {
const params = new SvelteURLSearchParams('foo=1');
const log: any = [];
const cleanup = effect_root(() => {
render_effect(() => {
const entries: string[] = [];
params.forEach((value, key) => entries.push(`${key}=${value}`));
log.push(entries.join('&'));
});
});
flushSync(() => {
params.set('foo', '2');
});
assert.deepEqual(log, ['foo=1', 'foo=2']);
cleanup();
});

@ -166,3 +166,77 @@ test('url.search normalizes value', () => {
test('SvelteURL instanceof URL', () => {
assert.ok(new SvelteURL('https://svelte.dev') instanceof URL);
});
test('url.searchParams subscribers are not notified by changes that leave the search string untouched', () => {
const url = new SvelteURL('https://svelte.dev/a?foo=bar');
const log: any = [];
const cleanup = effect_root(() => {
render_effect(() => {
log.push(url.searchParams.toString());
});
});
flushSync(() => {
// does not affect the search string
url.pathname = '/b';
});
flushSync(() => {
// neither does this
url.href = 'https://svelte.dev/c?foo=bar#hash';
});
flushSync(() => {
// but this does
url.search = '?foo=baz';
});
assert.deepEqual(log, ['foo=bar', 'foo=baz']);
cleanup();
});
test('url.searchParams.size is not notified by unrelated href changes', () => {
const url = new SvelteURL('https://svelte.dev/?foo=bar');
const log: any = [];
const cleanup = effect_root(() => {
render_effect(() => {
log.push(url.searchParams.size);
});
});
flushSync(() => {
url.href = 'https://svelte.dev/other?foo=bar';
});
flushSync(() => {
url.searchParams.append('baz', 'qux');
});
assert.deepEqual(log, [1, 2]);
cleanup();
});
test('url.searchParams.forEach re-runs when the search string changes via the URL', () => {
const url = new SvelteURL('https://svelte.dev/?foo=1');
const log: any = [];
const cleanup = effect_root(() => {
render_effect(() => {
const entries: string[] = [];
url.searchParams.forEach((value, key) => entries.push(`${key}=${value}`));
log.push(entries.join('&'));
});
});
flushSync(() => {
url.href = 'https://svelte.dev/?bar=2';
});
assert.deepEqual(log, ['foo=1', 'bar=2']);
cleanup();
});

Loading…
Cancel
Save