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 ziggy location instead of base url #1078

Merged
merged 1 commit into from
Jun 28, 2022
Merged

Fix ziggy location instead of base url #1078

merged 1 commit into from
Jun 28, 2022

Conversation

matthieumota
Copy link
Contributor

I've noticed that using the URL property to handle the active route on Ziggy in SSR is not correct. Indeed, we need to know the current URL and the URL property returns the base URL of the application.

To reproduce the bug, just disable the JS and find that the active route in the menu does not work. With JS, it works because the client re-hydrates the server-generated DOM.

So, I think we need to get current URL via request and totally replace window.location so.

@driesvints
Copy link
Member

This recently got worked on by @prestonholt at #1069. @prestonholt was this working for you before?

@matthieumota
Copy link
Contributor Author

Yes I see that but usage of page.props.ziggy.url is not correct and this is what I make this PR.

@prestonholt
Copy link
Contributor

This recently got worked on by @prestonholt at #1069. @prestonholt was this working for you before?

@driesvints I just tested and this wasn't working before #1069 either- this PR appears to fix the issue both pre and post #1069 changes.

I also tried fixing by following @aarondfrancis method, but it isn't fixing the route().current() issue either.

It seems like this PR fixes it because it keeps ziggy.url as the base URL, and also passes the current URL as ziggy.location, but I'm unsure whether ziggy.url needing to be the base URL is intended or a bug

@matthieumota
Copy link
Contributor Author

ziggy.url is only needed to generate correct routes with base URL. You can see that like a base URL, nothing more. That's why we need to get current location with Laravel Request and ziggy.location doesn't exists in native object. I've just added that to ziggy property. It is more simple like that.

@timacdonald
Copy link
Member

timacdonald commented Jun 28, 2022

It seems to me that this PR indeed creates parity between the non-SSR and SSR versions when calling route().current()

I've confirmed that before this tweak, route().current() did not work in SSR mode, but with the tweak things are now working as expected.

Test route().current() and route().current('route-name-here') work as expected.

Ziggy supports passing the "location" via config, and this value is used as a fallback when window === undefined, as is the case in SSR mode, thus this seems like a great solution and indeed the intended way to handle things.

Thanks @matthieumota for your PR!!

@timacdonald
Copy link
Member

Regarding ziggy.url it seems to me that if you set this to anything other than the base url, it is added to every URL generation, thus I believe that "url" config option is synonymous with "baseUrl" in this context.

@timacdonald
Copy link
Member

I've opened a matching PR for breeze: laravel/breeze#163

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.

5 participants