Skip to content
This repository has been archived by the owner on Feb 10, 2019. It is now read-only.

Add setup method to field #374

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tylergets
Copy link

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.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 70.69% when pulling 924bfa1 on tylergets:feature/setup_method into 171631a on Folkloreatelier:develop.

@mfn
Copy link

mfn commented Aug 30, 2018

I find this weird.

You are effectively duplicating the functionality of resolve itself, but:

  • calling it earlier
  • discard any return value

image

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.

@mfn
Copy link

mfn commented Aug 30, 2018

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.

@tylergets
Copy link
Author

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.

@mfn
Copy link

mfn commented Aug 31, 2018

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.

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

Successfully merging this pull request may close these issues.

3 participants