[LiveComponent] Fix loadingDirectives match elements from nested components #1392

smnandre · 2024-01-13T21:09:31Z

Q	A
Bug fix?	yes
New feature?	no
Issues	Fix #1382
License	MIT

The elements with loading directives were used in all LiveController they were in.

This PR fixes that and restrict loadingDirective to the scope of the LiveContoller (and filter those belonging to nested / child components)

src/LiveComponent/assets/dist/live_controller.js

weaverryan

Thanks for getting on this. Possible to add a test too - should be able to combine something from child.test.js into loading.test.js.

weaverryan · 2024-01-17T16:36:27Z

src/LiveComponent/assets/src/Component/plugins/LoadingPlugin.ts

+        let matchingElements = [...element.querySelectorAll('[data-loading]')];
+
+        // ignore elements which are inside a nested "live" component
+        matchingElements = matchingElements.filter((elt) => elt.closest('[data-controller~="live"]') === element);


We have a built-in mechanism for this we should use - it's elementBelongsToThisComponent from dom_utils.ts. We can pass the component instance from LoadingPlugin.attachToComponent into the callbacks, then into here so we can call that.

I think using the DOM will be better in term of performance, as we just manipulate Node element.. but that could be challenged. Do you want to use it for now ?

We could probably use only a CSS selector but i think we need to define this browser compatibility list before :)

Test added @weaverryan

Thanks for adding the test! I still think we should use elementBelongsToThisComponent(). By not using it, we're adding "dangling code": we have just one part of the code that works differently than everywhere else. We would need a real, verified performance reason to do such a thing. And, except for cases where there actually are children (which is not most live components), the elementBelongsToThisComponent() is basically just a call to element.contains() - https://github.com/symfony/ux/blob/2.x/src/LiveComponent/assets/src/dom_utils.ts#L204

Made changes, i cannot test locally for now, is this how you imagined it ?

WebMamba · 2024-01-29T10:24:04Z

src/LiveComponent/assets/dist/live_controller.js

@@ -2393,7 +2393,8 @@ class LoadingPlugin {
    }
    getLoadingDirectives(element) {
        const loadingDirectives = [];
-        let matchingElements = element.querySelectorAll('[data-loading]');
+        let matchingElements = [...element.querySelectorAll('[data-loading]')];
+        matchingElements = matchingElements.filter((elt) => elt.closest('[data-controller~="live"]') === element);


I am not sure about the use of the closest function. closest finds only one element, but in a live component you can have multiple elements with a loading attribute in nested element

closest is an ascending selector, it look the first parent "live controller"

weaverryan · 2024-01-29T14:54:03Z

src/LiveComponent/assets/src/Component/plugins/LoadingPlugin.ts

+        let matchingElements = [...element.querySelectorAll('[data-loading]')];
+
+        // ignore elements which are inside a nested "live" component
+        matchingElements = matchingElements.filter((elt) => elt.closest('[data-controller~="live"]') === element);


Thanks for adding the test! I still think we should use elementBelongsToThisComponent(). By not using it, we're adding "dangling code": we have just one part of the code that works differently than everywhere else. We would need a real, verified performance reason to do such a thing. And, except for cases where there actually are children (which is not most live components), the elementBelongsToThisComponent() is basically just a call to element.contains() - https://github.com/symfony/ux/blob/2.x/src/LiveComponent/assets/src/dom_utils.ts#L204

weaverryan

One minor tweak needed - thanks!

weaverryan · 2024-01-30T14:18:26Z

src/LiveComponent/assets/src/Component/plugins/LoadingPlugin.ts

-        component.on('loading.state:started', (element: HTMLElement, request: BackendRequest) => {
-            this.startLoading(element, request);
+        component.on('loading.state:started', (element: HTMLElement, request: BackendRequest, component: Component) => {
+            this.startLoading(component, element, request);


We already have the component as the argument to attachToComponent and we're attaching the loading.state.started to that specific component. So o need for the new argument to the callback: we can use the component variable already

As i could not test, i did it in a defensive way :)

I'll remove it

Fix / rebased / rebuilded (style cannot test locally -- will fix this next week)

Think i did what you meant but not sure

…onents

weaverryan · 2024-01-30T15:36:17Z

Thanks Simon!

smnandre · 2024-01-30T15:42:12Z

Oh damn... i was pushing to rebase .... hope i did not mess on the go with things

weaverryan · 2024-01-30T16:23:50Z

All good, I merged before you rebased. I did the rebasing ;)

…ase 🚀 (weaverryan) This PR was squashed before being merged into the 2.x branch. Discussion ---------- [Live][Stimulus] Prepping the LiveComponent Stable Release 🚀 | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Issues | None | License | MIT Hi! LiveComponents has, really, been quite stable for a long time, but it's kept its experimental status. Removing that is really about deciding that we'll protect backwards-compatibility. It's time to do that :). This is planned as the 2.15.0 release near the end of Feb (assuming we get the items below done before then). TODOs: * #1418 * Possibly remove Twig 2.x compat * #1428 * #1392 * Moving `Idiomorph` to a peer dependency would be great, but blocked by bigskysoftware/idiomorph#35 - **still need a tag** for the PR merge * #1426 as it may include some edge-case BC breaks. If there's anything else on your mind before stable, now is the time to mention it :). Cheers! Commits ------- 7932a9d [Live][Stimulus] Prepping the LiveComponent Stable Release 🚀

norkunas reviewed Jan 14, 2024

View reviewed changes

src/LiveComponent/assets/dist/live_controller.js Outdated Show resolved Hide resolved

weaverryan reviewed Jan 17, 2024

View reviewed changes

smnandre requested a review from weaverryan January 18, 2024 22:47

weaverryan mentioned this pull request Jan 28, 2024

[Live][Stimulus] Prepping the LiveComponent Stable Release 🚀 #1419

Merged

WebMamba reviewed Jan 29, 2024

View reviewed changes

weaverryan requested changes Jan 29, 2024

View reviewed changes

weaverryan added Bug Bug Fix Status: Needs Work Additional work is needed labels Jan 29, 2024

smnandre force-pushed the fix/loading-directives-parent-child-components branch 2 times, most recently from cbf694f to c769ac4 Compare January 29, 2024 20:43

smnandre requested a review from weaverryan January 29, 2024 21:22

weaverryan requested changes Jan 30, 2024

View reviewed changes

carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Jan 30, 2024

smnandre force-pushed the fix/loading-directives-parent-child-components branch 2 times, most recently from 1da752b to af3882c Compare January 30, 2024 15:02

smnandre requested a review from weaverryan January 30, 2024 15:04

[LiveComponent] Fix loadingDirectives match elements from nested comp…

7867e44

…onents

weaverryan force-pushed the fix/loading-directives-parent-child-components branch from af3882c to 7867e44 Compare January 30, 2024 15:36

weaverryan merged commit ed57804 into symfony:2.x Jan 30, 2024

Uh oh!

[LiveComponent] Fix loadingDirectives match elements from nested components #1392

[LiveComponent] Fix loadingDirectives match elements from nested components #1392

Uh oh!

Conversation

smnandre commented Jan 13, 2024

Uh oh!

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Jan 30, 2024

Uh oh!

smnandre commented Jan 30, 2024

Uh oh!

weaverryan commented Jan 30, 2024

Uh oh!

Uh oh!