Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: allow custom output via formatters #483

Merged
merged 13 commits into from
Mar 4, 2022

Conversation

jaulz
Copy link
Contributor

@jaulz jaulz commented Dec 3, 2021

This PR will let developers define custom templates for the different parts that can be generated. I am not sure about the template syntax (i.e. ":key") but that's the Laravel translation syntax.

This PR will let developers define custom formatters for the different parts that can be generated.

@jaulz jaulz force-pushed the feat/add-templates branch from 087b13c to 12dd749 Compare December 3, 2021 07:35
@jaulz jaulz changed the title feat: allow custom output via templates [WIP] feat: allow custom output via templates Dec 3, 2021
@jaulz jaulz changed the title [WIP] feat: allow custom output via templates [WIP] feat: allow custom output via formatters Dec 3, 2021
@jaulz jaulz changed the title [WIP] feat: allow custom output via formatters feat: allow custom output via formatters Dec 3, 2021
@jaulz
Copy link
Contributor Author

jaulz commented Dec 3, 2021

@bakerkretzmar I am not sure why the tests fail since I do not have the PHP version locally and there are no error messages in the output. With php@8.0 all tests are running fine:
image

@bakerkretzmar bakerkretzmar self-assigned this Dec 3, 2021
@bakerkretzmar
Copy link
Collaborator

@jaulz I think the test failures were because of the string typed properties, which weren't in PHP 7.3.

This is a really cool idea... I'm going to think about it for a bit 😄 I like the flexibility it adds but I'm not sure how many people would actually use it. Can you give some more examples of what you're hoping to do? The example in the test you added just leaves out the typeof window !== 'undefined' && typeof window.Ziggy !== 'undefined' check, which doesn't feel worth it to me.

Thanks!

@jaulz
Copy link
Contributor Author

jaulz commented Dec 4, 2021

The idea that floats in my head is to also expose the payload schema to have local validations (as far as it's possible) and thus this would be the initial feature that would allow me to hook into the generation of the route definitions. I am not yet sure how it will look like and when I will work on it but it would be handy to be ready at any time :)

@Tofandel
Copy link
Contributor

Awesome idea and PR, I would for sure use it

I just have one remark for the test unit though, instead of modifying the existing test, just copy it and then apply your modification to avoid 2 features overlapping in the same test

@jaulz
Copy link
Contributor Author

jaulz commented Feb 18, 2022

Yeah, I could do that but I don't want to spend any time on it as long as @bakerkretzmar does not consider it to be merged. Just let me know in case it gets relevant 😊

Copy link
Collaborator

@bakerkretzmar bakerkretzmar left a comment

Choose a reason for hiding this comment

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

Okay looking at this again, I like it a lot, let's do it 😎 couple questions before I take a final pass and merge it in:

  • I think I have a slight preference for resolving the formatters out of the application container, instead of listing classes in the config. So rather than config()->get('ziggy.formatters.file', FileFormatter::class), something like app(FileFormatter::class, $ziggy). We could register our default ones in our service provider, and then to override them, users would bind their custom ones in their own service provider instead of publishing and filling in our config file. What do you think of that approach? Any drawbacks you can see?
  • I think it would be cool if we didn't need to call ->format() and the Script classes were stringable/htmlable. In theory they could then go directly in Blade templates and they would get cast to strings/HTML during render. This feels a tiny bit more flexible to me, and basically makes the formatters more into 'payload objects' that could do formatting or potentially other stuff too. I don't mind making this change myself. Maybe I'm over thinking this, curious to know what you think though!

@jaulz
Copy link
Contributor Author

jaulz commented Feb 21, 2022

Great to hear 😊 Regarding your points:

@jaulz
Copy link
Contributor Author

jaulz commented Mar 1, 2022

Nice, that looks good 👍

@bakerkretzmar
Copy link
Collaborator

@jaulz thought about it some more and decided to keep your original implementation and get the classes from the config. Thanks for your patience!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants