Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert Don't init trees within ignore elements (x-ignore) #4471

Closed
wants to merge 1 commit into from

Conversation

danie-ramdhani
Copy link

Reason:

in async alpine plugin docs (https://async-alpine.dev/docs/upgrade/#html-attributes):

x-ignore will now be automatically added to all components.

this breaking async alpine data because after livewire doing server request then morph the view, x-ignore attribute will be added automatically by async alpine.

here's the simple example:

<div x-load x-load-src="/toggle.js" x-data="simpleToggle">
    <button class="rounded-2xl bg-slate-800 p-2" type="button" x-on:click="open = !open">
        Toggle
    </button>

    <div x-show="open" x-cloak>foo</div>
    <div x-show="!open" x-cloak>bar</div>
</div>

toggle.js:

export default () => {
    return {
        open: false,
    };
};
Video_2024_12_07-4.webm

@SimoTod
Copy link
Collaborator

SimoTod commented Dec 6, 2024

That library is a third party and not really related to the official project so it should probably be adjusted in their repository.

@danie-ramdhani
Copy link
Author

danie-ramdhani commented Dec 6, 2024

That library is a third party and not really related to the official project so it should probably be adjusted in their repository.

but it's breaking change tho. even filament lock their livewire version: filamentphp/filament@0fd9af5 but let's see what mr caleb think about this.

@joshhanley
Copy link
Collaborator

@danie-ramdhani can you link to me the open issues for async alpine and filament, I can see in detail what this issue is causing for them? Thanks!

@danie-ramdhani
Copy link
Author

@danie-ramdhani can you link to me the open issues for async alpine and filament, I can see in detail what this issue is causing for them? Thanks!

uh, no, it's not async alpine plugin issue, it's works as expected before alpine version 3.14.3 / livewire 3.5.12 (and then broken like the video i uploaded) and i think filament lock the livewire version because they're using async alpine too (filamentphp/filament@0fd9af5)

@joshhanley
Copy link
Collaborator

joshhanley commented Dec 7, 2024

@danie-ramdhani I'm after issues that have been raised on the Filament repo and the Async Alpine repo that clearly describe the problem that they are having (and that led to the decision to lock down Filament) so I can investigate. We're not going to just revert changes without looking into it as this was applied for a reason. I'd rather investigate the issues these packages are having so we can see if there is a better solution.

@danie-ramdhani
Copy link
Author

if i comment this line in livewire.js

image

it's works again as expected.

Video_2024_12_08-1.webm

@danie-ramdhani
Copy link
Author

danie-ramdhani commented Dec 7, 2024

@danie-ramdhani I'm after issues that have been raised on the Filament repo and the Async Alpine repo that clearly describe the problem that they are having (and that led to the decision to lock down Filament) so I can investigate. We're not going to just revert changes without looking into it as this was applied for a reason. I'd rather investigate the issues these packages are having so we can see if there is a better solution.

hmm... i can't find the opened issue after the locked version just guessing it 🤣

@SimoTod
Copy link
Collaborator

SimoTod commented Dec 7, 2024

@danie-ramdhani It could be worth trying the latest version of Livewire as well, if you haven't already tried it. There were a couple of bugs around Alpine 3.14.3 that have been already fixed. One of them was affecting x-ignore and was actually caused by a different change.
Just to confirm it's still an issue and it's a different bug.

@danie-ramdhani
Copy link
Author

danie-ramdhani commented Dec 7, 2024

@danie-ramdhani It could be worth trying the latest version of Livewire as well, if you haven't already tried it. There were a couple of bugs around Alpine 3.14.3 that have been already fixed. One of them was affecting x-ignore and was actually caused by a different change. Just to confirm it's still an issue and it's a different bug.

i am using latest livewire version (and the bug still in there), that's why this PR is opened.

@SimoTod
Copy link
Collaborator

SimoTod commented Dec 7, 2024

Fair enough, thanks for confirming. In theory async alpine removes x-ignore when ready before initialising the component so it's a weird behaviour. I assume something is going on when interacts with Livewire.

Can you just confirm the async alpine version to try to replicate your initial example? I assume it's the latest one, right?

@danie-ramdhani
Copy link
Author

danie-ramdhani commented Dec 7, 2024

Fair enough, thanks for confirming. In theory async alpine removes x-ignore when ready before initialising the component so it's a weird behaviour. I assume something is going on when interacts with Livewire.

Can you just confirm the async alpine version to try to replicate your initial example? I assume it's the latest one, right?

i use the latest version (v2) and v1 is using x-ignore as well.

I assume something is going on when interacts with Livewire.

it's because when alpine morph the view/page (doing server request thing), the x-ignore is getting morphed too.

@joshhanley
Copy link
Collaborator

joshhanley commented Dec 7, 2024

@danie-ramdhani thanks for checking! So the change to x-ignore (PR #4428) was because adding elements inside an element with x-ignore on it were being initialised when they shouldn't.

As @SimoTod mentioned it looks like Async Alpine removes the x-ignore and then inits the tree, but that doesn't seem to be working properly for some reason. We'd be happy to merge a fix into Alpine/Livewire if there is a bug around that, so everything works happily

@SimoTod
Copy link
Collaborator

SimoTod commented Dec 7, 2024

@danie-ramdhani

it's because when alpine morph the view/page (doing server request thing), the x-ignore is getting morphed too.

yeah, but if you look at the code of the plugin (https://github.com/Accudio/async-alpine/blob/main/src/async-alpine.js#L160), it removes x-ignore when the element is loaded and if x-ignore is not in the original template, it shouldn't really be added from nowhere when morping the page so the real issue is that an x-ignore attribute is added when in reality is not supposed to be there.

x-ignore marks a portion of html that should not run alpine so it would be good to know why x-ignore is still there.
Beside that, the template doesn't even need it, el._x_ignore is enough to guarantee what they are trying to achieve.

@SimoTod
Copy link
Collaborator

SimoTod commented Dec 7, 2024

@danie-ramdhani I'm 90% sure it's a bug on the async plugin when used with livewire (which was working before due to a bug in Alpine which was allowing directives inside an x-ignore to be processed).

I've added a couple of breakpoints on the JS library and, on refresh it runs the sync handler first which is supposed to check if el._x_async exists and, if not it adds the x-ignore stuff (which is correct).

https://github.com/Accudio/async-alpine/blob/316af0e5c178d9734cdb97fcc8c9772a21c0d36c/src/async-alpine.js#L80

After the library is compiled though, it gets translated in a funny way with only the first line being ignored so when the synch handler runs again, it's still adding x-ignore (but it shouldn't have done it). This was incorrect.

Screenshot 2024-12-07 at 23 20 38

This is the execution interrupted after the refresh (unfortunately is minified but you can recognise the syncHandler): instead of terminating immediately, it continues to the following lines when re-add the x-ignore stuff.

I assume Livewire is reinitialising the tree before the actual swap but, because of the async nature of the plugin. it may run the morphing before the async handler runs (at that point the element is swapped back with the previous metadata so it never runs the part that would re-remove x-ignore on the page element, the directives get picked by Alpine as a new one and eventually re-adds _x_ignore).

I thing x-ignore is not necessary, el._x_ignore = true should already do what needed and without the attribute, the livewire morphing/diffing may work just fine.

Tagging @Accudio for visibility.

@SimoTod
Copy link
Collaborator

SimoTod commented Dec 8, 2024

@danie-ramdhani when you have time, can you try this version of Async alpine and let me know if it fixes your issue: https://cdn.jsdelivr.net/gh/SimoTod/async-alpine@refs/heads/bug/x-ignore-readded-after-morphing/dist/async-alpine.script.js

The solution is to use skipDuringClone so x-load doesn't partially rerun on clone (it doesn't need to, data was already downloaded and injected in the component).

I'll test it properly tomorrow and if it works as expected, I'll open an issue on async-alpine, link a PR and ask their team to have a look.

Thanks

@danie-ramdhani
Copy link
Author

danie-ramdhani commented Dec 8, 2024

@danie-ramdhani when you have time, can you try this version of Async alpine and let me know if it fixes your issue: https://cdn.jsdelivr.net/gh/SimoTod/async-alpine@refs/heads/bug/x-ignore-readded-after-morphing/dist/async-alpine.script.js

The solution is to use skipDuringClone so x-load doesn't partially rerun on clone (it doesn't need to, data was already downloaded and injected in the component).

I'll test it properly tomorrow and if it works as expected, I'll open an issue on async-alpine, link a PR and ask their team to have a look.

Thanks

oh, it works as expected and fix the issue, thanks!
i think we can close this PR.

@Accudio
Copy link

Accudio commented Dec 8, 2024

Nice one, thank you @SimoTod! If you submit a PR or issue I'll look into and get it sorted asap!

@SimoTod
Copy link
Collaborator

SimoTod commented Dec 8, 2024

Yep. I just want to test a few cases properly before starting the conversation on your repo to make sure I don't break other things. (I'm actually off to Edinburgh tomorrow so I have a couple of hours on the train to test it, I believe you live there @Accudio).

@danie-ramdhani danie-ramdhani deleted the revert-4428 branch December 8, 2024 19:26
@joshhanley
Copy link
Collaborator

Thanks @SimoTod!

@SimoTod
Copy link
Collaborator

SimoTod commented Dec 9, 2024

@joshhanley No worries. Raised Accudio/async-alpine#47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants