CrowdStrike / ember-headless-form

Headless forms with a11y and validation support built in
https://ember-headless-form.pages.dev/
MIT License
29 stars 9 forks source link

Issues resetting form data #117

Closed averydev closed 1 year ago

averydev commented 1 year ago

Hi there!

I'm having an issue resetting / changing the form data values.

My use case is a form and a list, where after you add an item via a HeadlessForm, it is added to the list, the values are cleared, and you can create a new item.

Ideally either there would be a yielded method to change the data, or mutating the data via another approach would effect the data in the form.

simonihmig commented 1 year ago

I agree we need to support this, will look into this!

eelke commented 1 year ago

I noticed that resetting the form (using a simple <input type='reset'>Reset</input> also does not reset Yup validation errors. (This might be worth it's own issue?)

simonihmig commented 1 year ago

(This might be worth it's own issue?)

Yup (pun intended): #122

simonihmig commented 1 year ago

I worked on an elaborate solution for this, until I realized it actually does not serve the use case. 🙈😅

So back to the drawing board, trying to properly design this first before implementing it...

For some additional context: headless-form takes the approach of being strict about DDAU, even though in the case of forms there are some more or less valid cases for allowing two-way bindings maybe. But we don't do this, so the user will only get the new data by receiving it from @onSubmit. That means we have the original (unaltered) data coming from the caller as @data, and we have an internal data object that tracks the "dirty" data (user has filled in some fields, making those diverge from @data). What is displayed in the form is then basically the union of these, with any dirty value overriding the original one. Resetting the form then basically means resetting the internal data to an empty object, so all of @data is visible again (for forms used for "create" operations this is mostly just an empty object).

Im trying to incorporate the different cases of the state of the input (pristine - has never been touched by the user - vs. dirty - user has entered a different value than the initial one) and the action that is happening (already taking into account #122 here in the last two columns). Here is an initial suggestion for further discussion:

@data.foo is updated (@tracked) @data is reassigned input[type=reset] clicked yielded reset action called
input[name=foo] is pristine input is updated with @data.foo input is updated with @data.foo input is updated with @data.foo input is updated with @data.foo
input[name=foo] is dirty input is not updated (keeps filled in value) ? input is updated with @data.foo input is updated with @data.foo

I think we don't want the user provided value to be lost when @data.foo changes, right? So that's why we keep the value in col 1, row 2. Cols 3+4 should be uncontroversial, as these are explicit reset actions, so losing the existing values is expected.

I am unsure about col 2, row 2: when reassigning @data, should we keep existing user-entered values, or should we treat this as a "reset". In the latter case, we would have the use case of the original issue here covered. To erase all inputs you would do something like this.data = {};. But I am not sure if and how this is possible to implement, distinguishing this from col 1 (just changing a tracked prop), without resorting to weird observer-like patterns. But that's the second question, more important first is: do we want this? Is this reasonable? Or is it too inconsistent (with col 1), unexpected or too "magical"?

If we don't do this (i.e. keep the dirty value), we would still have no way to programmatically reset the form "from the outside". Another pattern that would allow this, which is not listed in the table above, is "Action down": letting the form call some kind of register callback, that gives the parent a reference to a reset-action that they can call (instead of relying on data to just flow down). PowerSelect uses a similar concept. Tbh I am not really a fan of it in general, as it can cause a bunch of problems (does not automatically account for the lifecycle of a component, so you can end up with some kind of "dangling pointer" to a reset action of an already destroyed component (that doesn't get garbage collected while the pointer exists)...

Thoughts? /cc @ynotdraw @nicolechung @NullVoxPopuli

simonihmig commented 1 year ago

Btw, there was an existing test covering some parts of what is mentioned above, but which was skipped for now, as indeed the logic we want here was not quite clear yet. So I think it's good to have this issue raised here, so we can get back to this!

nicolechung commented 1 year ago

Ideally either there would be a yielded method to change the data, or mutating the data via another approach would effect the data in the form.

This ask above sounds related to this comment below:

If we don't do this (i.e. keep the dirty value), we would still have no way to programmatically reset the form "from the outside". Another pattern that would allow this, which is not listed in the table above, is "Action down": letting the form call some kind of register callback, that gives the parent a reference to a reset-action that they can call (instead of relying on data to just flow down).

Am I reading this correctly?

nicolechung commented 1 year ago

Looks like Angular has a markAsPristine value where the form can be reset to @data even if @data has changed (been reassigned).

So in the case of input[name=foo] is dirty, markAsPristine would override the dirty value and reassign @data.

If markAsPristine hasn't been set, it's assumed the form is still "dirty" and so the new @data won't override.

Another option seems to be being able to pass the form value to reset. That way a form can be pristine but updated with the new @data in that way.

simonihmig commented 1 year ago

Am I reading this correctly?

Hm, not sure, I'll try to clarify this...

Ideally either there would be a yielded method to change the data

So I would read this as the last option (column) in my table above: "yielded reset action called", as in:

<HeadlessForm @data={{this.data}} as |form|>
  {{! form stuff }}
  <button type="button" {{on "click" form.reset}}>Reset</button>
</HeadlessForm>

Here we are yielding an action reset, that is called inside of the form block. The example is basically the equivalent of using a native reset button (col 3), as in:

<HeadlessForm @data={{this.data}} as |form|>
  {{! form stuff }}
  <button type="reset">Reset</button>
</HeadlessForm>

This is easy to do, as yielding down something will make it available to the block, and if that's where we want to use it, then that's fine.

A different case is yielding up. Like you want to call reset() inside say your controller (which renders the form). We don't have a natural way to do this. We just let data flow down, but we don't call functions "down". So this case was what I referred to in

Another pattern that would allow this, which is not listed in the table above, is "Action down": letting the form call some kind of register callback, that gives the parent a reference to a reset-action that they can call (instead of relying on data to just flow down).

An example of this would be:

// template.hbs
<HeadlessForm @data={{this.data}} @registerForm={{this.registerForm}} as |form|>
  ...
</HeadlessForm>
// controller.js

export default class FormController extends Controller {
  data = {};

  formApi;

  @action
  registerForm(formApi) {
    this.formApi = formApi;
  }

  someAction() {
    // do something, like saving a record based on the form data, and then resetting the form...

    this.formApi.reset();
  }
}

Here, with this.formApi.reset(), we are calling a function provided by the child form, but from upstream!

Again, I don't really like this very much, and would rather avoid this where possible, but it is an option on the table...

Regarding your references to Angular, I don't know enough about Angular to see how that could translate to Ember 😅

Another option seems to be being able to pass the form value to reset. That way a form can be pristine but updated with the new @data in that way.

Not sure what means exactly. Can you show or link to an example?

nicolechung commented 1 year ago

Another option seems to be being able to pass the form value to reset. That way a form can be pristine but updated with the new @Data in that way.

Maybe we can do something a bit like what ember-headless-table does and mix it with what react-hook-form does.

References ember-headless-table: useTable: https://github.com/CrowdStrike/ember-headless-table/blob/4f148be4f2270e8575c01cf3a98e18ee91bcf40d/ember-headless-table/src/-private/js-helper.ts#L18 react-hook-form: https://react-hook-form.com/api/useform/reset/

reset could take two options:

export default class FormController extends Controller {
const { reset } = useForm();

someAction() {
    // do something, like saving a record based on the form data, and then resetting the form...
   const values = await save('some-api', formValues);
    reset(values, { keepDirtyValues: true });
  }
}

I don't really know enough Angular either, so sticking with the react-hook-form as an example.

I don't have suggestions for how this would work with the current implementation however - how would the useForm hook know that it's related to <HeadlessForm />?

nicolechung commented 1 year ago

Unless we can do something like

<HeadlessForm @data={{this.data}} as |form|>
  {{! form stuff }}
  <button type="button" {{on "click" this.handleReset form.reset}}>Reset</button>
</HeadlessForm>

Where the form.reset is passed as an argument to handleReset...

simonihmig commented 1 year ago

Yeah, but this works only if the users interaction originated from within the form.

Let's give an example where this does not work: say we have a list of records and a sidebar. when you click a record, it will be highlighted in the list, and a form with the records data will show up in the sidebar, to allow editing it. Now if the user has entered some new data there, but didn't submit it, but instead selected a different record, we'll want to show that record's data in the form. But we also don't want the dirtied fields to preserve their value, as those were related to a totally different record. Se we want to discard all temporary changes in the form, and let the form just show the new @data. But how to tell that to the form? The action that caused the need to reset the form happened outside of the form (clicking the other list item), so we cannot pass form.reset along...

simonihmig commented 1 year ago

Also, we can look at this problem from two angles. First is how to support the use case of resetting the form. But whatever we come up with, we still have to also answer the question what the form should do in those cases that are contained in that table above: given we have dirtied fields, what should happen with those in case of 1) updating @data.foo and 2) reassigning @data? This needs to be answered anyway, independently of whether that'll be part of supporting the reset use case or not!

So the question there is basically: what would a user (you!) expect?

simonihmig commented 1 year ago

Maybe we can do something a bit like what ember-headless-table does and mix it with what react-hook-form does.

This got me thinking...

I don't have suggestions for how this would work with the current implementation however - how would the useForm hook know that it's related to ?

Yeah, that is the tricky part, how to make this work with Ember, where we don't have hooks, but we "only" have some data flowing down. We still want to call a reset() function that does something with the form component. This is still basically the "Actions down" approach. But maybe there is a way to support this, without the caveats of the register API that I mentioned above...

Picking up your example, and modifying it a bit to fit how this could work with Ember:

import { useForm } from 'ember-headless-form';

export default class FormController extends Controller {
  formController = useForm();  
  data = {};

  someAction() {
    // do something, like saving a record based on the form data, and then resetting the form...
    const values = await save('some-api', formValues);
    this.formController.reset();
  }
}
<HeadlessForm @data={{this.data}} @controller={{this.formController}} as |form|>
  ...
</HeadlessForm>

Ignore the hard part of naming things here (e.g. "controller"), that remains to be seen... but how it basically could work is that useForm() returns an object. The only public API on that object would be .reset() for now (and yeah, we could introduce options here and whatnot, but let's keep the example simple for now). But it could have additional data or methods, that only our component would be eligible to use. When passing this as @controller (or whatever better name we come up), the component would register itself with that object, and upon destruction unregister itself. This is similar to that registerAPI approach mentioned above, but without actually exposing these shenanigans to the user, and also with properly supporting the teardown case (no "dangling pointers"). The .reset() method would then know about the registered form component on that instance of the object, and be able to call "into" our component ("downwards").

It's gotten late over here, and I need to sleep over this, but to me this looks like an pretty interesting approach, that I didn't have on my mind before. Thanks a bunch for bringing this up @nicolechung!

ynotdraw commented 1 year ago

I'm a bit late, but after catching up - I think what you landed on above @simonihmig (with @nicolechung ) looks great to me. Also exposing it via the yielded form (form.reset) makes sense to me as well!

averydev commented 1 year ago

A couple of thoughts...

ember-basic-dropdown does a nice job of yielding actions that impact the component as a a hash of actions like:

yield (hash actions=(hash resetData=this.reset))

and you can pass those actions as arguments to other functions like:

<button {{on "click" (fn this.resetData dd.actions.reset)}}>

I like the registerForm approach you mentioned pretty well!

<HeadlessForm @data={{this.data}} @registerForm={{this.registerForm}} as |form|>
averydev commented 1 year ago

I would say though that in my view, here's what i would expect to happen.

<button {{on "click" this.resetData}}/>
<HeadlessForm @data={{this.data}} @registerForm={{this.registerForm}} as |form|>
...
</HeadlessForm
component
...
@action
resetData(){
this.data = {}
}

If that reset button is clicked, the data that's passed into the form has now changed and the form should respect the data that was passed down into it. Thats (to me) how "Data Down" would ideally be reflected here. The form takes whatever data it's given and presents it, then calls an action to do something to act on it.

When that wasn't how it worked, I was pretty surprised and assumed it was a bug.

I think it's totally fine if the new object clobbers the old -- the form wasn't submitted, so the changes are thrown away.

simonihmig commented 1 year ago

@averydev so what you are saying is that you would expect this to be our behavior, right? (pay attention to cols 1+2, row 2)

@data.foo is updated (@tracked) @data is reassigned input[type=reset] clicked yielded reset action called
input[name=foo] is pristine input is updated with @data.foo input is updated with @data.foo input is updated with @data.foo input is updated with @data.foo
input[name=foo] is dirty input is updated with @data.foo) input is updated with @data.foo input is updated with @data.foo input is updated with @data.foo

Means, whenever something changes around @data, the filled in (but not yet submitted) values will get wiped away. Is that what you would expect? Is that what others would also expect?

herzzanu commented 1 year ago

Nice ideas above. WDYT about the idea of passing the resetForm method as the second argument to the onSubmit action? The consumers will only be able to call resetForm and the reset would be done internally. Something like:

<HeadlessForm @data={{this.formData}} @onSubmit={{this.handleSubmit}}>
</HeadlessForm>
@action
handleSubmit(data, resetForm) {
  // save the data
  resetForm();
}
simonihmig commented 1 year ago

@herzzanu yeah, this would work, but only if resetting happens when (after) submitting. In this comment I gave an example of a use case that would also require resetting unrelated to submission. And ideally we would have a single API that covers both cases.

herzzanu commented 1 year ago

Right, I missed that. In that case, I guess this example you mentioned, makes even more sense.

herzzanu commented 1 year ago

On the other side, do you see a downside of a mixed solution between what I suggested above and your example? We know most of the time reset would happen after the form is submitted, so maybe we should offer an easy, straight forward way to call reset(). Then for any other exceptional cases we can offer the api similar to what you mentioned 🤔

NullVoxPopuli commented 1 year ago

There is this option? Always available to the style of api headless form provides (and maintainers don't need to change what is supported):

<HeadlessForm @data={{this.data}} as |form|>
  {{ ( this.assignFormApis form )}}
</HeadlessForm
assignFormApis(form) {
  this.resetForm = form.reset
}
simonihmig commented 1 year ago

The discussion here fluctuated a bit between supporting the specific use case of resetting (e.g. by calling some kind of reset action down) and the question of what should in general happen when @data changes while the form is displayed (and may have changed fields). But I think before we come up with a good API for the former, we actually have to answer the latter question. So to focus on that one first, I created this separate issue: https://github.com/CrowdStrike/ember-headless-form/issues/130. Please everybody involved here share your thoughts there! 🙏

herzzanu commented 1 year ago

@simonihmig I assume we can resolve this one based on the latest changes.

simonihmig commented 1 year ago

Yeah, I think so. I wanted to provide some examples of resetting forms in the docs, and demonstrate the different use cases. But no need to wait if you want to try that. 1.0.0-beta.3 has been released yesterday!