symfony / ux

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

[LiveComponent] data-loading behaviour not working correctly in child components #1382

Closed manuelkiessling closed 7 months ago

manuelkiessling commented 8 months ago

I'm trying to add the loading state behaviour documented at https://symfony.com/bundles/ux-live-component/current/index.html#targeting-loading-for-a-specific-action to my application.

I think I'm running into a bug that is not specific for my setup (PHP 8.2.14, Symfony 5.4.33, symfony/ux-live-component 2.13.3).

I have a component that looks like this:

Class:

#[AsLiveComponent(
    'test_child',
    '@test_child.component.html.twig'
)]
class TestChildComponent extends AbstractController
{
    use DefaultActionTrait;

    #[LiveAction]
    public function slowOperation(): void
    {
        sleep(2);
    }
}

Template:

<div {{ attributes }}>
    <a
            data-action="live#action"
            data-action-name="slowOperation"
    >
        Click me...
    </a>

    <div data-loading="action(slowOperation)|show">Loading...</div>
</div>

This works as expected when embedding the component into a template which is served through a "normal" Controller action:

Adobe Express 2024-01-08 09 58 01

(Please see https://www.dropbox.com/scl/fo/cifcwzq66p5fkxmw0lsh6/h?rlkey=h2o2z1753sdoqeqhis17vqf3n&dl=0 for hi-resolution versions of the screen recordings)

The "loading" message briefly flashes while the page and its component is being loaded, and is then only shown while the request for the "slowOperation" action is running, being removed when this has finished — as expected.

This behaviour becomes broken when I put the TestChildComponent into a TestParentComponent, like this:

Parent class:

#[AsLiveComponent(
    'test_parent',
    '@test_parent.component.html.twig'
)]
class TestParentComponent extends AbstractController
{
    use DefaultActionTrait;
}

Parent template:

<div {{ attributes }}>
    <twig:test_child />
</div>

My assumption is that this should not change the behaviour in any way, but it does: Adobe Express 2024-01-08 09 59 35

As before, there is only a brief flash of the loading message upon full-page-load. However, when clicking the button and thus triggering the action, the loading message is shown but remains shown, even after the action/request has finished.

Only subsequent clicks then result in the correct behaviour.

I'm seeing even wilder misbehaviour in my actual code, where the element with the data-loading attribute is shown (and remains shown) even when I'm only leaving a form field and no action is triggered at all.

weaverryan commented 8 months ago

Hey!

Would it be possible for you to publish your reproducer as a GitHub repo that I could clone quickly to check things out?

Thanks!

manuelkiessling commented 8 months ago

Hey @weaverryan,

I've set up https://github.com/manuelkiessling/symfony-ux-issue-1382-reproducer, which reliably reproduces the issue with the latest-and-greatest versions of all involved components.

Running

git clone https://github.com/manuelkiessling/symfony-ux-issue-1382-reproducer.git \
  && cd symfony-ux-issue-1382-reproducer \
  && composer install \
  && nvm install \
  && npm install --force --no-save \
  && npm run dev \
  && symfony server:start

You can then open http://127.0.0.1:8000/reproducers/working and http://127.0.0.1:8000/reproducers/not-working to see the two behaviours.

smnandre commented 8 months ago

Well it's probably related to the way we register those "data-loading" elements, as we do not take account of wether they are nested or not..

So we could(should) keep only the "direct descendant" of the component, and ignore the "grand-children" like in your example...

... but that would probably make some bug reports as soon as we release it, no ? I mean it's a "bug", but fixing it can have big impact... wdyt @weaverryan ?

smnandre commented 8 months ago

Side note: great repro, thank you! 🎩

smnandre commented 8 months ago

@manuelkiessling could you try this PR and confirm it fixes the bug ?

manuelkiessling commented 8 months ago

@smnandre I'm afraid I'm failing to switch the dependencies of my project to your fork&branch with the fix.

I've done the following changes: https://github.com/manuelkiessling/symfony-ux-issue-1382-reproducer/commit/b0f47b6376b32d44155a3d795f88e758a0c8ad12

However, a subsequent composer update symfony/ux-live-component symfony/ux-twig-component results in:

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires symfony/ux-live-component dev-fix/loading-directives-parent-child-components, found symfony/ux-live-component[v2.0.0, ..., 2.x-dev] but it does not match the constraint.
  Problem 2
    - Root composer.json requires symfony/ux-twig-component dev-fix/loading-directives-parent-child-components, found symfony/ux-twig-component[v2.0.0, ..., 2.x-dev] but it does not match the constraint.

I'm not too experienced with juggling Composer dependencies, so if you have a hint for me, that would be much appreciated.

smnandre commented 8 months ago

Well, let's see when it's released then :) But i really do thing it's fixed, as i reproduced locally and then saw the fix working :)