Skip to content

Commit 6bde3b6

Browse files
Rich-HarrisRich-Harris
authored
fix: snapshot form fields on read (#16150)
At the moment there's a big ol' `$state` object backing a form object's `fields`, which means the whole thing is mutable. As such you can end up with surprising results: ```js myform.enhance(async (form) => { const data = form.fields.value(); try { await form.submit(); } catch (e) { showErrorUi(e, data.whatever); } }) ``` Inside the `catch` clause, `data.whatever` might be the value that was submitted, or it might be the value as it is _now_, if it was changed while the submission was happening. This PR fixes it. --- ### Please don't delete this checklist! Before submitting the PR, please make sure you do the following: - [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] This message body should clearly illustrate what problems it solves. - [x] Ideally, include a test that fails without this PR but passes with it. ### Tests - [x] Run the tests with `pnpm test` and lint the project with `pnpm lint` and `pnpm check` ### Changesets - [x] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running `pnpm changeset` and following the prompts. Changesets that add features should be `minor` and those that fix bugs should be `patch`. Please prefix changeset messages with `feat:`, `fix:`, or `chore:`. ### Edits - [x] Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.
1 parent 9161740 commit 6bde3b6

5 files changed

Lines changed: 130 additions & 1 deletion

File tree

‎.changeset/green-beds-arrive.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@sveltejs/kit': patch
3+
---
4+
5+
fix: snapshot form fields on read

‎packages/kit/src/runtime/form-utils.js‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -607,6 +607,34 @@ function get_type_prefix(field_type, is_array, input_value) {
607607
return '';
608608
}
609609

610+
/**
611+
* A deep-clone implementation specifically for form data, where
612+
* we don't need to worry about cycles and whatnot
613+
* @param {any} value
614+
* @returns {any}
615+
*/
616+
function deep_clone(value) {
617+
if (value !== null && typeof value === 'object') {
618+
if (value instanceof File) {
619+
return value;
620+
}
621+
622+
if (Array.isArray(value)) {
623+
return value.map(deep_clone);
624+
}
625+
626+
/** @type {Record<string, any>} */
627+
const clone = {};
628+
for (const key of Object.keys(value)) {
629+
clone[key] = deep_clone(value[key]);
630+
}
631+
632+
return clone;
633+
}
634+
635+
return value;
636+
}
637+
610638
/**
611639
* Creates a proxy-based field accessor for form data
612640
* @param {any} target - Function or empty POJO
@@ -618,7 +646,8 @@ function get_type_prefix(field_type, is_array, input_value) {
618646
*/
619647
export function create_field_proxy(target, get_input, set_input, get_issues, path = []) {
620648
const get_value = () => {
621-
return deep_get(get_input(), path);
649+
const value = deep_get(get_input(), path);
650+
return deep_clone(value);
622651
};
623652

624653
return new Proxy(target, {

‎packages/kit/test/apps/async/src/routes/remote/form/snapshot/+page.svelte‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
<script lang="ts">
2+
import { update, release } from './snapshot.remote.ts';
3+
4+
let status = $state('idle');
5+
let captured = $state<string | undefined>('none');
6+
let live = $state<string | undefined>('none');
7+
</script>
8+
9+
<form
10+
{...update.enhance(async (form) => {
11+
// take a snapshot of the fields *before* doing any async work
12+
const data = form.fields.value();
13+
14+
// the submission is held open by the server, giving the test a window
15+
// to mutate the form state after the snapshot has been taken
16+
status = 'submitting';
17+
await form.submit();
18+
status = 'done';
19+
20+
// the snapshot should reflect the values at the time it was taken, not
21+
// any changes to the form that happened post-submission
22+
captured = data.a?.b?.c;
23+
live = form.fields.a.b.c.value();
24+
})}
25+
>
26+
<input {...update.fields.a.b.c.as('text')} />
27+
<button>submit</button>
28+
</form>
29+
30+
<form {...release}>
31+
<button>release</button>
32+
</form>
33+
34+
<p id="status">status: {status}</p>
35+
<p id="captured">captured: {captured}</p>
36+
<p id="live">live: {live}</p>

‎packages/kit/test/apps/async/src/routes/remote/form/snapshot/snapshot.remote.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
import { form, getRequestEvent } from '$app/server';
2+
import * as v from 'valibot';
3+
4+
const deferreds: Array<PromiseWithResolvers<void>> = [];
5+
6+
export const update = form(
7+
v.object({
8+
a: v.object({
9+
b: v.object({
10+
c: v.string()
11+
})
12+
})
13+
}),
14+
async (data) => {
15+
// hold the submission open until `release` is called, so that the test
16+
// can mutate the form state while the submission is in flight
17+
if (getRequestEvent().isRemoteRequest) {
18+
const deferred = Promise.withResolvers<void>();
19+
deferreds.push(deferred);
20+
await deferred.promise;
21+
}
22+
23+
return data.a.b.c;
24+
}
25+
);
26+
27+
export const release = form(v.object({}), () => {
28+
for (const deferred of deferreds) {
29+
deferred.resolve();
30+
}
31+
deferreds.length = 0;
32+
});

‎packages/kit/test/apps/async/test/test.js‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -651,6 +651,33 @@ test.describe('remote functions', () => {
651651
]);
652652
});
653653

654+
test('form.fields.value() returns an immutable snapshot in an enhance callback', async ({
655+
page,
656+
javaScriptEnabled
657+
}) => {
658+
if (!javaScriptEnabled) return;
659+
660+
await page.goto('/remote/form/snapshot');
661+
662+
await page.fill('input[name="a.b.c"]', 'original');
663+
await page.getByRole('button', { name: 'submit' }).click();
664+
665+
// wait until the snapshot has been taken and the submission is in flight
666+
await expect(page.locator('#status')).toHaveText('status: submitting');
667+
668+
// mutate the form state *after* the snapshot was taken
669+
await page.fill('input[name="a.b.c"]', 'changed');
670+
671+
// let the submission complete
672+
await page.getByRole('button', { name: 'release' }).click();
673+
await expect(page.locator('#status')).toHaveText('status: done');
674+
675+
// the captured snapshot must not reflect the post-submission change...
676+
await expect(page.locator('#captured')).toHaveText('captured: original');
677+
// ...while reading the field again reflects the current state
678+
await expect(page.locator('#live')).toHaveText('live: changed');
679+
});
680+
654681
test('nested field set is SSR rendered', async ({ page }) => {
655682
await page.goto('/remote/form/set-ssr');
656683
await expect(page.locator('#description')).toHaveText('Description: nested');

0 commit comments

Comments
 (0)