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

Consolidate payload generation into new Ziggy class #305

Merged
merged 29 commits into from
Jul 24, 2020

Conversation

bakerkretzmar
Copy link
Collaborator

This PR extends and closes #272, extracting all the logic related to generating Ziggy's 'route payload' directly into a new Ziggy class that replaces the existing RoutePayload. It also removes the static compile method in favour of instantiating a new Ziggy object, and resolves the application router instance internally so it doesn't need to be passed in.

The new Ziggy class also implements JsonSerializable, which means we can json_encode() it directly—this makes the use case in the original PR, returning Ziggy's payload as JSON directly from an endpoint, trivial:

return response()->json(new Ziggy('group'));

This PR contains significant breaking changes and will be included in Ziggy 1.0. All code like RoutePayload::compile($router) will need to be replaced with something like (new Ziggy)->toArray() or (new Ziggy)->toArray()['namedRoutes'].

@DanielCoulbourne if you have some time to take a look at this, I'd love to know if it's more or less what you were going for!

Copy link
Collaborator

@jakebathman jakebathman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One change and a question. Good work! Approved for merge after that.

tests/Unit/MacroTest.php Outdated Show resolved Hide resolved
src/Ziggy.php Outdated Show resolved Hide resolved
@bakerkretzmar bakerkretzmar merged commit 45025ae into develop Jul 24, 2020
@bakerkretzmar bakerkretzmar deleted the jbk/extract-payload branch July 24, 2020 14:43
Copy link
Collaborator

@jakebathman jakebathman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good with this now that tests are all passing 👍

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

Successfully merging this pull request may close these issues.

2 participants