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.

Sign up for GitHub

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

Jump to bottom

Ziggy is no longer optional #1023

Closed
flick36 opened this issue Mar 29, 2022 · 5 comments · Fixed by #1024
Closed

Ziggy is no longer optional #1023

flick36 opened this issue Mar 29, 2022 · 5 comments · Fixed by #1024
Labels
bug

Comments

@flick36
Copy link

flick36 commented Mar 29, 2022 •
edited
Loading

  • Jetstream Version: 2.7.0
  • Jetstream Stack: Inertia
  • Uses Teams: no
  • Laravel Version: 9.6.0
  • PHP Version: 8.1.4
  • Database Driver & Version: Postgres 14.2

Description:

Well with the newly added Intertia SSR Support #1012 Ziggy is now imposed to us

jetstream/src/Http/Middleware/ShareInertiaData.php

Line 10 in 090fdfb

use Tightenco\Ziggy\Ziggy;

jetstream/src/Http/Middleware/ShareInertiaData.php

Lines 60 to 63 in 090fdfb

'ziggy' => function () {
return (new Ziggy)->toArray();
},
]));

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

jetstream/src/Console/InstallCommand.php

Line 284 in 090fdfb

$this->requireComposerPackages('inertiajs/inertia-laravel:^0.5.2', 'tightenco/ziggy:^1.0');

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:

jetstream/stubs/inertia/resources/js/ssr.js

Lines 18 to 27 in 090fdfb

.mixin({
methods: {
route: (name, params, absolute) => {
return route(name, params, absolute, {
...page.props.ziggy,
location: new URL(page.props.ziggy.url),
});
},
},
});

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 #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
Copy link

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
Copy link
Member

Ping @xiCO2k

@xiCO2k
Copy link
Contributor

xiCO2k commented Mar 30, 2022 •
edited
Loading

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
Copy link
Member

@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.

@driesvints driesvints added the bug label Mar 30, 2022
@xiCO2k
Copy link
Contributor

xiCO2k commented Mar 30, 2022

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.

@xiCO2k xiCO2k mentioned this issue Mar 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[2.x] Not require Ziggy on ShareInertiaData middleware. xiCO2k/laravel-jetstream
4 participants
@driesvints @xiCO2k @flick36 @timrspratt