symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
820 stars 297 forks source link

[LiveComponent] [Form] Sometimes save function doesn't have correct value from the last edited form field #1333

Closed ypaquereau closed 8 months ago

ypaquereau commented 9 months ago

Hello :wave:

I try to use LiveComponent for some forms, but sometimes I got a weird behavior

When I fill one form field, and just after click on the Submit button, sometimes the updated value from the edited field isn't sent, and keep the previous value.

In my network tab, I see that I got the validation ajax call for my field, and before its finished, I got my Save ajax call
The save live action does not wait the validation ajax call to finish network

I record a little video, where I try to update the quote field value, but keep the old one after I submit the form
live_component_race_demo

I also made a simple reproducer repository : https://github.com/gantiapp/live-component-race-issue

weaverryan commented 9 months ago

Hi there!

I checked this out, but I still can't reproduce it. I believe that even in your reproducer, the issue happens only sometimes, right?

In my network tab, I see that I got the validation ajax call for my field, and before its finished, I got my Save ajax call The save live action does not wait the validation ajax call to finish

In your screenshot, what does the green vs blue line mean? It looks like the save Ajax call waits for the green, but slightly overlaps the blue. What is the blue? In LiveComponents, on the frontend, we purposely make sure that we wait for 1 Ajax call to finish before starting the next. In the compiled file, you can see that logic here - https://github.com/symfony/ux/blob/2.x/src/LiveComponent/assets/dist/live_controller.js#L1991-L1994

I can clearly see the issue from your video, but I don't yet understand it. To make things stranger, by the time the first AJAX request finishes, the "model" should have been updated on the client-side. This means that, even if the save AJAX call did start early, it should be sending the correct model data.

Do you have situations when where this happens more often?

ypaquereau commented 9 months ago

Hi again :wave:,

I can confirm that it only happens sometimes.
In our team, all of the members (3 people) are able to reproduce the issue, but not at the same frequency (1 almost always, 1 almost never, and for me sometimes).

In my screenshot, the green is the time it waits before getting a response, and the blue part is the time where is downloading it (maybe more info in this screenshot)
image

When I arrive in the form, I have this HTML for the quote form field :

<textarea id="admquote_quote" name="admquote[quote]" required="required" class="form-control">Houston, we have a problem.</textarea>

and when I edit this field, I got from the ajax validation response this HTML :

<textarea id="admquote_quote" name="admquote[quote]" required="required" class="form-control">Houston, we have a</textarea>

I think it used the HTML text value when we submit the form, so I don't know if when it's waiting that the validation ajax call is finished, it also waits that the HTML is replaced

Also, when we try to reproduce in our team project (which is very big), we reproduce the issue more often

To test if it's an overlap issue, I add in my save action, I add a debounce.
With it, I never reproduce the problem :

{{ form_start(form, { attr: {
    'data-action': 'live#action',
    'data-action-name': 'prevent|debounce(1000)|save',
}}) }}
ypaquereau commented 9 months ago

Also, if I use the network throttling in Firefox with the Good 2G, I reproduce almost every time
With that, I also saw that the save ajax call is launched before the validation ajax call is finished

Screenshot from 2023-12-20 09-51-36

smnandre commented 9 months ago

I have a genuine doubt about it: if (and i’m curious to know) the second one seems to start before the « end » of the first, can it be because we just use the HTTP status code in the first case, so we dont wait the full transfer of the answer ?

weaverryan commented 9 months ago

Oh that’s a great theory…

weaverryan commented 8 months ago

I'm not so sure about the status code theory now: we wait for the response.body. To help debug, anyone experiencing this (since it's hard to reproduce, even with the reproducer) could hack some code into live_controller.js:

Or, perhaps even better, if you are able to find some way to reproduce it more consistently. For example, if you add a usleep() into the __invoke() method of DefaultActionTrait, does that help? That would cause the re-render (from the model change) to be delayed, but would not impact the performance of the save LiveAction.

smnandre commented 8 months ago

we wait for the response.body.

No we don't. Or more precisely we don't wait enough to block any other request to start.

You spotted the await, but it's almost made ineffective by the line just before, that seems to control the sequentiallity of requests

this.isRequestPending = false;
this.backendRequest.promise.then(async (response) => {
+    this.backendRequest = null;
    const backendResponse = new BackendResponse(response);
    const html = await backendResponse.getBody();
tryStartingRequest() {
    if (!this.backendRequest) {
        this.performRequest();
        return;
    }
    this.isRequestPending = true;
}

And tryStartingRequest is called by render() that is called by... many thing :)

weaverryan commented 8 months ago

Fixed in #1411