laravel-graphql
laravel-graphql copied to clipboard
Add setup method to field
Adding a setup method allows people to load models/resources once instead of multiple times. If you need to verify authorization via a field on a model you later need in resolve, you can load it in the setup method as a field on the mutation/query.
I can provide an example if the usefulness of this is not clear.
Coverage increased (+0.2%) to 70.69% when pulling 924bfa11673ef4a8b0a42678f3ef854224e0e383 on tylergets:feature/setup_method into 171631a17e4bf33780513f15221ea6052c67d498 on Folkloreatelier:develop.
I find this weird.
You are effectively duplicating the functionality of resolve
itself, but:
- calling it earlier
- discard any return value
Adding a setup method allows people to load models/resources once instead of multiple times.
I've solved this by not using authorized
or authenticated
; just resolve
.
If you do this, you're back to where you are before your PR: you've a single method, can load all the resources you need, can (re-)use them for your authorization/authentication, have them available for resolving and you don't need a dedicated field on your type for it.
TL;DR: I had the same problem until I realized the approach by the graphql wrapper only has downsides and simply didn't make use of it. Adding yet another method won't make the mess ... less messier :-)
I did try to resist my approach because of (so it seemed) clean separation of the action methods but in reality I simply do have overlapping code (performance optimization) for authentication/resolve so it made sense to ignore those additional magic methods.
Another thing: last time I checked, you only have one instance per GraphQL type class. Thus a field containing loaded resources are effectively shared resources unless you're observant about this.
That for me was the No. 1 reason not to go with this "property caching" approach.
We need to call it before any of the other methods so that it can set up properties for the models used
I find the methods to be quite handy as it prevents having to import the exceptions. Are the authorize/authenticated methods planning on being deprecated? I think a single method is better, however, I think in that case there needs to be a cleaner way to trigger errors.
You are correct when it comes to types but not mutations/queries, in that case, we could switch this to only function in Mutations/Queries similar to shouldValidate. I don't think this is really a problem though.
I find the methods to be quite handy as it prevents having to import the exceptions. … I think in that case there needs to be a cleaner way to trigger errors.
Can you expand on this?
Are the authorize/authenticated methods planning on being deprecated?
If you watch the repo closely you will realize it's currently in an somewhat abandoned state with no actions from repo owners at all.
You are correct when it comes to types but not mutations/queries
Ah, thanks, I may have been misguided here!
Still, I resorted to only call resolve
and be done with it. The additional methods only introduce unnecessary overhead for me.