Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use:enhance resets form even if form.x values used for populating it #8513

Closed
dummdidumm opened this issue Jan 13, 2023 · 27 comments · Fixed by #9267
Closed

use:enhance resets form even if form.x values used for populating it #8513

dummdidumm opened this issue Jan 13, 2023 · 27 comments · Fixed by #9267
Labels
bug Something isn't working forms Stuff relating to forms and form actions ready to implement please submit PRs for these issues!
Milestone

Comments

@dummdidumm
Copy link
Member

dummdidumm commented Jan 13, 2023

My understanding is the form will take any values that are set by the server; if I remove use:enhance from the form in this repo, I get the same values after POST that I do before. But adding use:enhance changes this behaviour. What am I missing here?

Originally posted by @angrytongan in #8404 (comment)

Not sure what's the best way to respond here. One way would be to check the form value that is returned, and if it contains any keys, assume it may be used for form data, and don't reset in that case. It's an imperfect solution but probably the best we can do. (It's also a confusing solution maybe, so maybe it's better to not reset after all..? could be seen as a breaking change though)

@dummdidumm dummdidumm added bug Something isn't working forms Stuff relating to forms and form actions labels Jan 13, 2023
@angrytongan
Copy link
Contributor

I would argue that server gets final say - matches non-JS behaviour, and I think submission of a form doesn't mean that the client should automatically clear it. Definitely a breaking change, however the reset() workaround you mentioned is good. Happy to go either way 👍

@eltigerchino
Copy link
Member

eltigerchino commented Jan 15, 2023

Is it possible for the default behaviour to call form.reset before the DOM is updated? Then, the form is still populated with any returned form values from the server after resetting.

Hacked together this ugly demo: https://stackblitz.com/edit/github-akyafk?file=src/routes/+page.svelte

I'm still thinking about it but it's likely this solution has some downsides to it or may not even be possible. Wondering how to access the beforeUpdate lifecycle function from the enhance action...

EDIT: this solution seems to still reset the values of previous values if you submit it a second time

@eltigerchino
Copy link
Member

eltigerchino commented Jan 16, 2023

The input also does not reset as long as a new or different value is submitted. However, submitting the same value consecutively guarantees a reset.

https://stackblitz.com/edit/github-akyafk?file=src/routes/+page.svelte

The issue is input.value not updating properly. After the form is reset, input.value should be populated by the form value returned when applyAction is called. However, the new input value is not reflected in the reactive statement or the paragraph in the demo above.

@BMorearty
Copy link

I came here just now to post a very similar issue with use:enhance but it's probably the same issue so I'll post it as a comment here.

Repo with reproducible case

Instead of initializing the form from export let form, I'm initializing it from the data I returned in export let data.

The result is the same as in your repro case: the form behaves differently with use:enhance than without it. With use:enhance the input field is not correctly initialized.

@dummdidumm
Copy link
Member Author

Instead of initializing the form from export let form, I'm initializing it from the data I returned in export let data.

This is a good point - and a reason why my idea about not calling reset when form is empty wouldn't suffice. I think we're painted into a corner here, shit.

@geoffrich
Copy link
Member

If it's not possible to detect this at the framework level, could we potentially provide (or document) a custom enhance that does not reset the form? Then people who want to opt-out of this behavior could use that enhance everywhere.

It could look something like this in userland (though with this version, there's no way to opt out of update if you return a callback):

<script lang="ts">
	import { enhance } from '$app/forms';
	import type { SubmitFunction } from '$app/forms';

	function noResetEnhance(node: HTMLFormElement, cb?: SubmitFunction) {
		return enhance(node, async (initProps) => {
			const returnedCb = await cb?.(initProps);

			return (returnProps) => {
				returnedCb?.(returnProps);
				returnProps.update({ reset: false });
			};
		});
	}
</script>

@BMorearty
Copy link

Maybe I'm stating the obvious, but an ideal solution would replicate non-enhanced behavior--it would not try to avoid resetting the form, but instead reset it and then repopulate it using whatever logic populated it in the first place.

@BMorearty
Copy link

I've pushed an update to my reproducible case. It demonstrates that not only does the form not behave the same with/without use:enhance, so do other parts of the page.

My script has this:

  export let data;
  let city = data.city;

And I changed one part of the page content—outside the form—from:

<p>City is now {data.city}</p>

to

<p>City is now {city}</p>

After submitting the form with use:enhance, the city variable is not updated so it shows the old value.

This will be a real puzzler to solve, if it's even possible.

@eltigerchino
Copy link
Member

eltigerchino commented Jan 17, 2023

My script has this:

  export let data;
  let city = data.city;

And I changed one part of the page content—outside the form—from:

<p>City is now {data.city}</p>

to

<p>City is now {city}</p>

After submitting the form with use:enhance, the city variable is not updated so it shows the old value.

You’ll need to use a reactive statement such as:
$: city = data.city

The $: declaration causes city to be updated anytime data.city changes

@BMorearty
Copy link

@s3812497 Good point. The reactive statement does work around the issue.

I’d still consider it either a bug or maybe just a limitation to be documented, because the page behavior with vs. without use:enhance is different when the reactive $: statement isn't used. And the docs say use:enhance will emulate the browser-native behavior. But at least for my case it's an easy workaround and I'm satisfied with it. 👍🏻

@BMorearty
Copy link

Update: oh yeah, I remember there is a reason I didn't use $: in the first place.

If your goal is to initialize city from the load function and then bind it to the input after initial assignment, the reactive statement doesn't work because city is being bound to two things at once (data.city and the input value):

<script>
  import { enhance } from '$app/forms';
  export let data;
  $: city = data.city;
</script>

<form use:enhance>
  <input name="city" bind:value={city} type="text" />
  <input type="submit" />
</form>

The behavior is no characters appear when you type into the city field.

@eltigerchino
Copy link
Member

eltigerchino commented Jan 17, 2023

Update: oh yeah, I remember there is a reason I didn't use $: in the first place.

If your goal is to initialize city from the load function and then bind it to the input after initial assignment, the reactive statement doesn't work because city is being bound to two things at once (data.city and the input value):

<script>
  import { enhance } from '$app/forms';
  export let data;
  $: city = data.city;
</script>

<form use:enhance>
  <input name="city" bind:value={city} type="text" />
  <input type="submit" />
</form>

The behavior is no characters appear when you type into the city field.

Ah yeah, sorry. I was thinking of the one-way binding with value={city}
It properly updates the value whenever city changes but then you can’t easily use bind: to get the input.value when a user enters something

@dummdidumm
Copy link
Member Author

dummdidumm commented Jan 17, 2023

One idea I had to fix the OP's issue is to do this after form.reset():

for (const element of form.elements) {
  if (
    (element instanceof HTMLInputElement && element.type !== 'submit') ||
    element instanceof HTMLTextAreaElement ||
    element instanceof HTMLSelectElement
  ) {
    console.log('dispatch on', element.value);
    element.dispatchEvent(new Event('input', { bubbles: true }));
    element.dispatchEvent(new Event('change', { bubbles: true }));
  }
}

... but the input values are still lost if I submit the form twice in a raw without changing the values inbetween. Svelte rightfully says "hey from my perspective the value hasn't changed" so it doesn't run an update, things get out of sync and eventually the input value is lost.

@rsweeneydev
Copy link

I ran into this issue today as well.

FWIW, part of what I expect to be enhanced in a form by adding "use:enhance" vs a typical browser form submit is that my form fields won't be cleared. Since the page is not reloaded, page state should be preserved.

@dummdidumm
Copy link
Member Author

FWIW this was not reset by default originally, but many people complained that this doesn't match the native browser behavior, which is why we changed it but also added the option so that you can disable the reset.
Classic case of "can't make everyone happy".

@Rich-Harris
Copy link
Member

This seems to fix it — it's a tiny bit hacky but probably acceptable under the circumstances?

// client.js 
const props = {
-  form: result.data,
+  form: null,
  page: { ...page, form: result.data, status: result.status }
};
root.$set(props);
+root.$set({ form: result.data });

@dummdidumm
Copy link
Member Author

Could this have any unintended side effects like reactive statements running twice? I don't think so because it's batched for the next microtask.

@Rich-Harris
Copy link
Member

Reactive statements will run twice, but they need to be accounting for the null case anyway because that's the default state of form

@Rich-Harris Rich-Harris added the ready to implement please submit PRs for these issues! label Feb 3, 2023
@Rich-Harris Rich-Harris added this to the soon milestone Feb 3, 2023
@Rich-Harris
Copy link
Member

Rich-Harris commented Feb 3, 2023

Marking this ready to implement as I'm fairly confident the approach above will work (and the caveat around reactive statements is acceptable)

@PatrickG
Copy link
Member

I am not sure if this should be handled better in svelte.
https://discord.com/channels/457912077277855764/1071277537377931274/1071362560730873856

@BMorearty
Copy link

@PatrickG Any chance you could just post the relevant content here? Not everyone is on Discord. Thanks.

@PatrickG
Copy link
Member

@PatrickG Any chance you could just post the relevant content here? Not everyone is on Discord. Thanks.

The problem is, when svelte rerenders the value, it does not set the value of the input, because it thinks it has not changed, but the enhance action calls form.reset().
Svelte needs to set input.value again if input.value !== form.providedName

You can see it in the JS output on line 41 of this repl: https://svelte.dev/repl/40820395c601425aa27bed26d628c799?version=3.55.1

            if (dirty & /*form*/ 1 && input_value_value !== (input_value_value = /*form*/ ctx[0]?.providedName || '') && input.value !== input_value_value) {
                input.value = input_value_value;
            }

would need to be

            if (dirty & /*form*/ 1 && (input_value_value !== (input_value_value = /*form*/ ctx[0]?.providedName || '') || input.value !== input_value_value)) {
                input.value = input_value_value;
            }

or without the cache

            if (dirty & /*form*/ 1 && input.value !== (ctx[0]?.providedName || '')) {
                input.value = ctx[0]?.providedName || '';
            }

Normally svelte philosophy is "don't mess with the dom yourself". From this standpoint, it is correct to not set input.value again.
But here sveltekit is "messing" with the dom by calling form.reset()

BTW, you can workaround this with a two-way binding
See line 49 of the JS output of this repl: https://svelte.dev/repl/9554466f345c41739fee6e6c06ca4130?version=3.55.1

            if (dirty & /*providedName*/ 1 && input.value !== /*providedName*/ ctx[0]) {
                set_input_value(input, /*providedName*/ ctx[0]);
            }

Rich-Harris added a commit that referenced this issue Mar 2, 2023
* set `form` to `null` before updating it with real values - fixes #8513

* changeset and test
@stv8
Copy link

stv8 commented May 24, 2023

I came here just now to post a very similar issue with use:enhance but it's probably the same issue so I'll post it as a comment here.

Repo with reproducible case

Instead of initializing the form from export let form, I'm initializing it from the data I returned in export let data.

The result is the same as in your repro case: the form behaves differently with use:enhance than without it. With use:enhance the input field is not correctly initialized.

@BMorearty did you ever figure out a solution to this? I just ran into this same issue and I haven't found an easy way to repopulate the form with data returned by my load function.

@BMorearty
Copy link

@stv8 I came up with three workarounds. These are the notes I wrote to myself at the time:

  1. Don’t use use:enhance.
  2. Any time you assign a variable from a data field, make it reactive with a $:. Note that this doesn’t work if you bind the input to the variable, because then the variable is reactive to two sources: data and the input. E.g.,
<script>
  import { enhance } from '$app/forms';

  export let data;
  // Initial assignment on page load and reassignment on change
  $: city = data.city;
</script>

<p>City is now {city}</p>

<form method="POST" use:enhance>
  Change city to
  <!-- Can’t use bind:value here -->
  <input name="city" value={city} type="text" />
  <input type="submit" />
</form>
  1. Don’t use the bare default use:enhance. Pass a function that, after update, manually re-populates values that the rest of your component needs to display correct updated data. E.g.,
<script>
  import { enhance } from '$app/forms';

  export let data;
  // Initial assignment on page load
  let city = data.city;
</script>

<p>City is now {city}</p>

<form
  method="POST"
  use:enhance={() => {
    return async ({ update }) => {
      await update();
      // Redo the assignment from the script above
      city = data.city;
  };
}}>
  Change city to
  <input name="city" value={city} type="text" />
  <input type="submit" />
</form>

@stv8
Copy link

stv8 commented May 24, 2023

@BMorearty thanks, I thought option 3 was going to work for me but I'm still having issues. I think I'm going to go ahead and try out https://superforms.vercel.app instead.

@hyunbinseo
Copy link
Contributor

hyunbinseo commented Jan 17, 2024

I hope this behavior is documented somewhere.

I was having a hard time understanding why the form value was null, even after update().

<!-- src/routes/+page.svelte -->
<script lang="ts">
  import { enhance } from '$app/forms';
  import { tick } from 'svelte';
  export let form;
</script>

<form
  use:enhance={() => {
    return async ({ update }) => {
      await update();
      console.log(form); // null
      await tick();
      console.log(form); // { at: 1705498579836 }
    };
  }}
  method="post"
>
  <button>Submit</button>
</form>
// src/routes/+page.server.ts
export const actions = {
  default: () => ({ at: Date.now() })
};

Current documentation is misleading, since update is only a part of the default logic.

update is a function which triggers the default logic that would be triggered if this callback wasn't set


Update: result.data can be typed. #7161 (comment)

@cleytoncarvalho
Copy link

was this fixed? I still have the problem. Should I use update({ reset: false }) in all my forms that have data returned from the server?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working forms Stuff relating to forms and form actions ready to implement please submit PRs for these issues!
Projects
None yet
Development

Successfully merging a pull request may close this issue.