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

GraphQL Macroable #592

Merged
merged 6 commits into from
Feb 26, 2020
Merged

GraphQL Macroable #592

merged 6 commits into from
Feb 26, 2020

Conversation

stevelacey
Copy link
Contributor

@stevelacey stevelacey commented Feb 26, 2020

Summary

Adds macro support to the GraphQL service/facade. I am often finding myself wanting Arr/Str-esque helpers for GraphQL behaviours (like spinning up Type's), and given static calls to the GraphQL facade are already used in lots of places, this seems to fit well.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md

Let me know if you need anything I've crossed out.

@stevelacey
Copy link
Contributor Author

stevelacey commented Feb 26, 2020

A simple example of where this would be useful, I've inherited a GraphQL API where the type returned can change based on conditions (don't ask); the codebase has lots of this:

public function type() : Type
{
    return $this->hasPagination()
        ? GraphQL::paginate('User')
        : Type::listOf(GraphQL::type('User'));
}

It'd be great to wrap this up into a macro like this:

GraphQL::macro('paginateOrListOf', function (string $name) : Type {
    return Something::hasPagination()
        ? GraphQL::paginate($name)
        : Type::listOf(GraphQL::type($name));
});

So my queries can do:

public function type() : Type
{
    return GraphQL::paginateOrListOf('User');
}

Without having to share that logic via inheritence, traits, or some other class.

This allows the end user to expand upon the provided facade statics like GraphQL::type($name).

@mfn
Copy link
Collaborator

mfn commented Feb 26, 2020

It's my personal opinion that Macroable that way is an abomination for proper software development, i.e. what you said would be the "correct way"

Without having to share that logic via inheritence, traits, or some other class.

But I see the signs (read: pressure from the community) so I'm not vetoing it (just dying a little inside). So let's move forward 🕳

What you be so kind and:

  • mention it in the readme with some example code
  • add a test for it?
    I don't want to see myself trying to remove this in a commit silently without anyone noticing at 3am in the morning. 😏

Btw, very nice first contribution ❤️

@stevelacey
Copy link
Contributor Author

@mfn awesome 👏 I'll do those things, and your objection will be noted in the ship's log 🙃

@stevelacey
Copy link
Contributor Author

@mfn readme updated and test added. For the readme I copied and reworded https://laravel.com/docs/6.x/responses#response-macros

Also submitted some unrelated readme fixes that this is based against in #593

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

Thanks!

@mfn mfn merged commit 47bb5bc into rebing:master Feb 26, 2020
@stevelacey stevelacey mentioned this pull request Feb 26, 2020
1 task
@stevelacey stevelacey deleted the patch-1 branch February 26, 2020 12:23
@mfn
Copy link
Collaborator

mfn commented Apr 3, 2020

https://github.com/rebing/graphql-laravel/releases/tag/5.0.0 has been released which includes this!

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.

None yet

2 participants