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

Fix @teleport directive when view is cached #8282

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

luanfreitasdev
Copy link
Contributor

@luanfreitasdev luanfreitasdev commented Apr 10, 2024

Review the contribution guide first at: https://livewire.laravel.com/docs/contribution-guide

1️⃣ Is this something that is wanted/needed? Did you create a discussion about it first?
#8130
2️⃣ Did you create a branch for your fix/feature? (Main branch PR's will be closed)
Yes
3️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.
No
4️⃣ Does it include tests? (Required)
No
5️⃣ Please include a thorough description (including small code snippets if possible) of the improvement and reasons why it's useful.

When we run php artisan view:cache, the @teleport directive is broken. I didn't understand the reason in depth, however, when we create a directive similar to @this using the Facade Blade, the same problem does not occur.

  • I noticed how it is called to create the directive for @teleport differs from @this. For this reason, I left it the same as @this and it worked.

  • I was unable to create a test that runs view:cache and generates the cache for a dusk test at run time

@PerryvanderMeer
Copy link
Contributor

PerryvanderMeer commented Apr 10, 2024

The problem is caused by the fact that the @teleport directive is registered via app('livewire')->directive('teleport'), which calls app(ExtendBlade::class)->livewireOnlyDirective() via the LivewireManager. So, the directive is only available while rendering via Livewire's own blade compiler. Which is obviously not used when compiling and caching the views via Laravel's compiler.

You can test if the @teleport directive is also available while caching the views via Laravel's compiler with:

Blade::compileString(<<<'HTML'
    <div>
        @teleport('foo')
            bar
        @endteleport
    </div>
HTML);

It's more or less related to #8068.

@luanfreitasdev
Copy link
Contributor Author

@PerryvanderMeer, do you believe that this PR should not continue?

@PerryvanderMeer
Copy link
Contributor

Well it solves the problem, but I'm not sure why @calebporzio initially limited the scope of the directive to Livewire's blade compiler.
Anyhow, you could extend your PR with a test based on the example code that I've given above.

@luanfreitasdev
Copy link
Contributor Author

This doesn't seem to work very well with testing. I also tried creating a component with a blade file and clearing the cache using Artisan::call('view:cache') and the problem didn't happen.

@calebporzio
Copy link
Collaborator

Yeah, this is a problem with using app('livewire')->directive(). I initially introduced "livewire only" blade directives so that Livewire was less-invasive. But view caching kinda ruins that. I'm going to merge this and we should probably remove the concept of livewire-only Blade directives entirely. (This is the same problem we experience with the morph marker injection not happening when views are cached)

@calebporzio calebporzio merged commit 10bf80e into livewire:main Apr 18, 2024
3 of 4 checks passed
@MrPunyapal
Copy link

All models and slideovers which were in @teleport() behaving odd after this update!

They use to stay on screen on subsequent requests now they disappear after a subsequent request!

@joshhanley
Copy link
Member

@MrPunyapal Can you make a https://wirebox.app that demonstrates the issue?

@MrPunyapal
Copy link

MrPunyapal commented Apr 26, 2024

image

somehow it is not working for me
I can create repo if you want?

@MrPunyapal
Copy link

@joshhanley https://wirebox.app/ is not working for 3.4.11 only 👀 it works for 3.4.10!

@joshhanley
Copy link
Member

joshhanley commented Apr 27, 2024

@MrPunyapal bugger, thanks! Will see if I can get it fixed.

@joshhanley
Copy link
Member

@MrPunyapal wirebox should be fixed now 🙂

@MrPunyapal
Copy link

@MrPunyapal wirebox should be fixed now 🙂

thanks @joshhanley

see the bug

3.4.10: https://wirebox.app/b/xeo72

3.4.11: https://wirebox.app/b/430dl

open the modal and click on add random
both codes are exact the same!

@PerryvanderMeer
Copy link
Contributor

@MrPunyapal Your 3.4.10 Wirebox is private, so we can't access.

@MrPunyapal
Copy link

@MrPunyapal Your 3.4.10 Wirebox is private, so we can't access.

done

@PerryvanderMeer
Copy link
Contributor

PerryvanderMeer commented Apr 27, 2024

Tested a few things. Blade rendering and Livewire's update response are completely the same. Must be a problem since Alpine 3.13.9. Either way not related to this PR.

@MrPunyapal
Copy link

Tested a few things. Blade rendering and Livewire's update response are completely the same. Must be a problem since Alpine 3.13.9. Either way not related to this PR.

FYI: without @-teleport it has the same behavior on both versions!

@luanfreitasdev
Copy link
Contributor Author

What is causing this is Alpine, not the Blade directive. I made a version of the old directive with the new Alpine and it also fails

https://wirebox.app/b/4qp32

@MrPunyapal
Copy link

But it's 3.4.11?

@luanfreitasdev
Copy link
Contributor Author

Yes
image

@luanfreitasdev
Copy link
Contributor Author

Blade Directive with version 3.4.10 (works)

https://wirebox.app/b/x6djd

@MrPunyapal
Copy link

Blade Directive with version 3.4.10 (works)

https://wirebox.app/b/x6djd

Btw I forgot one thing!

I was having this in my app service provider!

I was using a blade directive from 3.3 only! 🙂

https://twitter.com/MrPunyapal/status/1746556766278897995

@MrPunyapal
Copy link

Solved it by tweaking some code at all the teleports

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.

None yet

5 participants