Accudio / async-alpine

Async Alpine brings advanced code splitting and lazy-loading to Alpine.js components!
https://async-alpine.dev
Apache License 2.0
152 stars 12 forks source link

Race condition with nesting #39

Closed allensquare closed 6 months ago

allensquare commented 7 months ago

Hello,

I'm experiencing a race condition issue with nested async alpine when processing mutations via the _mutations setup here. The problem surfaces when I try to reference a variable from the parent component in the child component.

Usually, the parent node is able to download the javascript and execute the x-data directive first. However, sometimes the child node executes first, leading to an error when attempting to access an unset variable. When I pause at the time of error with debugger and inspect the DOM, I notice that the child node has already had its x-ignore attribute removed, while the parent node still has its x-ignore attribute. If I continue with debugger, I can see that parent node then gets its x-ignore removed and x-data processed after.

I suspect that issue may be caused by _componentStrategy being async but not waited on, and so it can't guarantee that the parent node downloads the javascript and activates itself before its child components. Normally, x-ignore on any parent node should prevent any alpine directives from running in any child node, but because async alpine runs Alpine.initTree directly on the child node, it appears to ignore this.

Quick example of a setup, and let's imagine that component2.js references a variable that is set up in component1.js, such as this.component1property.

<div
    x-ignore
    ax-load
    ax-load-src="/assets/component1.js"
    x-data="component1"
>
     <div
        x-ignore
        ax-load
        ax-load-src="/assets/component2.js"
        x-data="component2"
    >
        ...
    </div>
</div>

I do see some historical implementation of some parent strategy that appears to deal with similar concerns, but looks like that has long been removed. Any thoughts?

iniznet commented 7 months ago

I can't reproduce your issue, it loads in order just fine in my simple parent-child component test with the default (eager) strategy.

You can employ the event strategy to ensure that the parent is always loaded first and then trigger an event that the child component listens to.

<div
    x-ignore
    ax-load
    ax-load-src="/assets/component1.js"
    x-data="component1"
    x-init="$dispatch('async-alpine:load', { id: 'component-2' })"
>
     <div
        x-ignore
        ax-load
        ax-load-src="/assets/component2.js"
        x-data="component2"
        id="component-2"
    >
        ...
    </div>
</div>

Or, using a custom event, with the parent: $dispatch('custom-event') and child: ax-load="event (custom-event)".

allensquare commented 7 months ago

@iniznet

Here's a quick example to replicate the bug: codesandbox

Try refreshing the page multiple times and watching the console errors. On good loads with no errors, clicking the increment button will increase both numbers. However, if you see the console error Uncaught ReferenceError: component1property is not defined and try to click the increment button, you'll see that only the first one increments.


The solution you proposed might work for the simple case, but imagine a case where you have a third level nested component component3 that relies on data from both component1 and component2 (more than a single parent). If both component 1 and component 2 dispatched async-alpine:load with { id: component-3 }, then component 3 would resolve as soon as either component 1 or 2 are ready, without ensuring both 1 and 2 are ready.

<div
    x-ignore
    ax-load
    ax-load-src="/assets/component1.js"
    x-data="component1"
    x-init="$dispatch('async-alpine:load', { id: 'component-3' })"
>
    <div
        x-ignore
        ax-load
        ax-load-src="/assets/component2.js"
        x-data="component2"
        x-init="$dispatch('async-alpine:load', { id: 'component-3' })"
    >
        <div
            x-ignore
            ax-load
            ax-load-src="/assets/component3.js"
            x-data="component3"
            id="component-3"
        >
            ...
        </div>
    </div>
</div>
allensquare commented 7 months ago

Basically the wrong _x_dataStack is being built during addScopeToNode for the child node. If you inspect the child node's properties, you'll see that it's missing parent's data sometimes.

Accudio commented 7 months ago

Hmmm good catch @allensquare.

You're correct in that a previous Async Alpine iteration could load components independently of their parent elements, by using a strategy of renaming Alpine attributes. I removed that functionality in version 2.0 in favour of relying on x-ignore for a simpler and faster implementation.

I think it's important to retain the asynchronous behaviour of component downloads to prevent waterfalls, but we definitely need to come up with a solution to only allow child components to run when they're able to.

Without digging into the code — I can't at the moment — I think the solution here is probably that we shouldn't run initTree if the current component has x-ignore in its ancestors. I think we should be able to register the component and remove x-ignore safely since the parent is still preventing Alpine from running it. Then when the parent component is initialised, it will run both itself and the child component.

I can probably look into this in more depth later this week, or could review a PR.

It is curious if initTree allows us to run a child component even with an x-ignored ancestor—it doesn't seem like intended functionality but I wonder if that would allow a reliable way to re-add the ability to load wrapper components independently of child components. With version 1.0 I did use that where different JS was required for different screen sizes but they could use the same markup. That's a separate conversation and experiment however!

Accudio commented 6 months ago

This has been merged and published in patch version 1.2.2.

Thank you @allensquare for raising the issue and @helio3197 for the Pull Request!