Form inputs are not updated on re-render

[dr-nafanya]

This looks like as a reincarnation of the issue #272

The issue is illustrated by the following failing test: master...bug-invalid-input-value-after-render

The test failure is caused by a bit controversial logic, that preserves the form input element values on rendering: https://github.com/walmartlabs/thorax/blob/master/src/form.js#L186-L199 While there’s a reasonable use case for this logic, the suggested test case also seems to be valid. It is also a valid expectation from the user’s standpoint to have the form rendered with the new values, rather than observing old values without an obvious reason.

What do you think about making this logic optional by default, i.e. allow a flag on the view that would explicitly tell that form data must be preserved?

/cc @kpdecker @blakeembrey

[blakeembrey]

Makes sense to me, but I haven't committed to the project for a year or so now.

I think if a user has specifically updated it, it should respect that and discard the old value. However, the logic exists to keep partially entered user input data around when we re-render for whatever reason. It'd be great for both functionality to kept. E.g. grab user input values, set with new options on render, values that haven't changed between renders can be repopulated with user data. Otherwise, a reset option for the render makes perfect sense and is easy to implement.

[dr-nafanya]

@blakeembrey thanks for quick reply!

Honestly I’m afraid to let Thorax make assumptions on how the form fields were populated - via previous rendering or were they changed by an user one way or another. I’d make rendering the view without populating form with previous data a default behavior, and allow user to explicitly specify if they want the form data to be preserved over the rendering.

[kpdecker]

General it's not recommended to use value= in thorax templates as they break due to the re-render restore logic. You can't have both and in the same manner that users expect the content to change when it's backing model changes, they also expect it to not change when they have entered data.

We can't differentiate between these two cases currently and simply avoiding the use of value= should be sufficient to work around any issues seen.

[jasonwebster]

@dr-nafanya: You can easily opt-out of populate being called by default by setting options: { populate: false } on your view subclass prototype.

But, @kpdecker is correct. The idiomatic Thorax way to simply allow populate to do its job and never use the value attribute in templates. (you may also have to set options: { populate: { context: true} } if you're expecting it to match context keys to form fields.

[kpdecker]

Sorry for not further updating this, I'm leaving this open as I want to beat on this feature a little bit and see if we are doing this in the most optimal way. If not too expensive, it would be better to make this a bit smarter. I.e. better detection of if the change comes from the user vs. the model. I think this can be done relatively cheaply (provided that serialize is used) but need to investigate further.