search

laravel / jetstream

Tailwind scaffolding for the Laravel framework.
https://jetstream.laravel.com
MIT License
3.97k stars 818 forks source link

Ziggy is no longer optional #1023

Closed flick36 closed 2 years ago

flick36 commented 2 years ago

Description:

Well with the newly added Intertia SSR Support https://github.com/laravel/jetstream/pull/1012 Ziggy is now imposed to us

https://github.com/laravel/jetstream/blob/090fdfbdf7f622836d2e6336f27f9749028247c2/src/Http/Middleware/ShareInertiaData.php#L10

https://github.com/laravel/jetstream/blob/090fdfbdf7f622836d2e6336f27f9749028247c2/src/Http/Middleware/ShareInertiaData.php#L60-L63

So i believe theres no more reason to install ziggy as an optional dependecy

https://github.com/laravel/jetstream/blob/090fdfbdf7f622836d2e6336f27f9749028247c2/src/Console/InstallCommand.php#L284

and it should now be part of the package, also i think this is a breaking chance (since it is) because y if people decide not to use ziggy because as the creator of Inertia Jonathan Reinink says :https://twitter.com/reinink/status/1298577581650980871 some of use get rid of Ziggy because it causes more issues than it solves. But now we are forced to. wich is ok, it's your package guys, thanks for all the work you. just, like I mentioned above, now require the package withing the package and don't make it optional if we are not able to remove it, or maybe make it so we can publish de middleware and we might if we choose get rid of it completley.

To add on that, i think it's even bad practice to be using ziggy the way the Pull Request uses it: https://github.com/laravel/jetstream/blob/090fdfbdf7f622836d2e6336f27f9749028247c2/stubs/inertia/resources/js/ssr.js#L18-L27 since, in Vue 3 mixins are no longer recommended https://vuejs.org/api/options-composition.html#mixins

And since in the same relase we just got the composition API https://github.com/laravel/jetstream/pull/1001 as default it does make sense not to force ziggy on us

Sorry for the probably bad english btw, not my first tongue

Steps To Reproduce:

Fresh install of Jetstream with the removal of Ziggy as dependency

timrspratt commented 2 years ago

I've just faced this issue also. I removed Ziggy because it felt like it exposed too much. This occurred with a composer update since the Ziggy package is not a dependency anymore:

Class "Tightenco\Ziggy\Ziggy" not found
driesvints commented 2 years ago

Ping @xiCO2k

xiCO2k commented 2 years ago

We can remove Ziggy, but that way we need to change all URL's need to be changed to not use the route() mixin and we need to set the url directly.

Ziggy was already a requirement before the addition of SSR. (now we just need to pass on both the <head> for browsers, and on the ShareInertiaData to the SSR.

driesvints commented 2 years ago

@xiCO2k the problem is that existing apps don't have it installed. And thus ShareInertiaData breaks since it's a package class and not a stub.

xiCO2k commented 2 years ago

Yes that's right, will PR a change on that later today, to move the Ziggy share to the HandleInertiaRequests stub.

That way the user have full control.