[Bug]-[540]:storeValue() drops values when called in quick succession

zzgvh commented 3 years ago

Description

I'm using appsmith.store to gather data that is ultimately used when posting a form. The store is also used to populate the form fields when certain events happen on the page.

However the use of the input fields that have onTextChanged events that call storeValue() suffer from a kind of slowness and ultimately dropped updates to the store it seems. This leads to values that you thought were entered being incorrect.

Note that I enter the number in the first line slowly and you can see how the store updates for each keystroke in the debug text field below the form. When I edit the second line to input 50 quickly the second storeValue() call seem to be dropped; you can see that the keystroke registers in the UI, but the update of the store seems not to happen leading to the input reverting the value 5 since that is the value of the store. This happens more easily in Firefox than in Chrome, but I can trigger it there too.

Demo app (rename to .json, Github doesn't allow the uplaod of JSON for some reason...)

Important Details

Version: Cloud / Self-Hosted
OS: I'm mostly on Linux. Mint 19.
Browser: I see this in both Firefox and Chrome on Linux
Environment production

Nikhil-Nandagopal commented 3 years ago

@zzgvh this is because the storeValue is async today. We'll be introducing a synchronous API which is non persistent

hetunandu commented 3 years ago

@zzgvh This is happening because onTextChange is called in a debounced fashion. It has nothing to do with storeValue from what I can see. In my testing, I saw that updates were delayed but the order was correct.

zzgvh commented 3 years ago

Hey @hetunandu. I don't quite understand what you mean by onTextChange being called in a debounced fasion. Is there anything I can do to fix or mitigate the problem?

hetunandu commented 2 years ago

Hey @hetunandu. I don't quite understand what you mean by onTextChange being called in a debounced fashion. Is there anything I can do to fix or mitigate the problem?

Sorry for the late reply

We debounce events from onTextChange as they could end up getting fired a lot. This is a platform feature and can't really be changed. Storing/processing every keystroke from input is not really possible today.

If you could explain the use case you are trying to solve here, maybe I can help you with an alternative solution @zzgvh

hetunandu commented 2 years ago

@zzgvh were you able to find another way to solve this? Let me know if I can help.

zzgvh commented 2 years ago

Hey again @hetunandu. It seems the performance of Appsmith is a little better, particularly in deployed mode, but I still see problems when I do complex calls involving the store in event handlers, like Input.onTextChanged. Here's an example of a handler that can't keep up:

{{(
function updatePicksEditor1() {
  function doStore(key, value) {
    const editor = appsmith.store?.picks?.editor;
    storeValue("picks", {
      ...appsmith.store?.picks,
      editor: [
        {
          ...editor[0],
          [key]: value,
        },
        // spread operator for array literals do nothing if it is an empty array
        ...(editor[1] ? [editor[1]] : []),
      ],
    });
  }
  doStore("Sälj", Math.max(0, parseInt(SalesCount1.text || "0")));
}
)()}}

This code lives in onTextChange of an Input widget set to the Number type. If I hit the up or down arrows repeatedly there will be missed clicks.

https://user-images.githubusercontent.com/142613/144475725-dab9bf9a-a426-4161-90de-5967803026e2.mp4

The vid shows the code above in action. First I go slow, four click up, this increases the value by four. Then four clicks down, value goes down as expected. Then I click up four times in quick succession. Only two register. Two more quick "four clicks" up. Two register each time. Then four clicks quickly down. They all go through. Finally four clicks up, only one of which registers.

I get the feeling that the events get mixed up time wise, so that early events actually fire after later ones. Just guessing of course, but if you look closely you can see the number go up and then down again when I hit up four times quickly.

I had hoped to be able to use the JS Editor to replace the store for these kinds of "page global data", by using the editor variables, but alas, that was not to be :cry: Seems like the JS editor variables cannot be changed and used by the page yet.

hetunandu commented 2 years ago

@zzgvh

In the first example I saw that you were writing and reading from the store in the same input. This, according to Appsmith is an antipattern.

Let's take an Input Widget for example. Features of the Input widget are:

Input widget has a text property that gets updated by the user typing in the input
The Input Widget will invoke onTextChange in a debounced fashion when ever the text changes
The Input Widget will update its text value whenever the defaultText value updates

The way you have set this input is such

onTextChange updates the store
the defaultText has the binding to the same store

So what ends up happening is this: -> User updates the text input in quick succession (assume they are typing "1234") -> The value is stored in the appsmith.store (since it is debounced only "12" went through) -> the input's default text gets updated because the store has been updated ( updates to "12") ( meanwhile the user finishes typing "1234" ) -> the input's text gets updated with the default text (which was "12") -> User lost the input as the new text property is "12" -> if the user were to continue typing, it would append on "12" to become "1256" ("34" event seems to have been dropped)

And that is why, we consider this as an antipattern. In reactive systems like Appsmith, if two different sources update a single value at the same time, race conditions would occur. In this case, the two sources are the user typing and the widget defaultText setting the text.

I advice that you refer to the Input.text property directly to make decisions and business logic and avoid going through the store for this. We guarantee that Input.text will always be accurate when a user is typing in the input. defaultText is mainly used to reset the text property to an older or new state and not used to actively manage the state of the Input widget.

Hope I could explain this issue and the cause of it properly here. If not, I am happy to join you on a call to provide more examples. Incase you want me to help you with a workaround to your current problem, I can help with that as well.

zzgvh commented 2 years ago

Hey @hetunandu. Thanks for the extensive description of how the system "steps on its own toes" :wink: I think I now understand why this issue happens. So my question becomes, is there any other way that you can change the value of an input widget programmatically while allowing the user to use the same widget?

In the video, you see two buttons below the input, with a "-" and a "+", that I demo the problem on. When these buttons are pressed the appsmith.store value driving the defaultText of the input is changed by one. The UI reason for this is that when working on a tablet "fat fingers" make it hard to use the arrows of the input itself. Another use case that I have in another view is when I have two inputs that show values based on the same underlying appsmith store value. And so on; I have quite a few places where I cannot use just the raw value of an input, since I need to be able to change it via "external user action".

As it stands I cannot see how to make these kinds of somewhat involved uses of inputs without relying on the store for the value to be represented, and the onChanged event and defaultText property of the input to change and display the value. And it seems to me to be necessary part of being able to build sophisticated and (hopefully) user friendly functionality.

If you can devise some other way to accomplish this I'd be more than happy to "change my ways"! And if there is no other way I'd really like this to be an improvement of Appsmith in the future.

Nikhil-Nandagopal commented 2 years ago

@zzgvh in this case, you can use the store to update the input's values but you don't need to write back to the store from the input when the input changes. I've detailed an example of this below. You can fork the app and check the code https://app.appsmith.com/applications/61dfc1b919ba421fdd1ce508/pages/61dfc1b919ba421fdd1ce50b

bharath31 commented 1 year ago

assuming 30% of users who use storeValue face this issue

Stats

Stat	Values
Reach	270
Effort (months)	0.5

appsmithorg / appsmith