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

Resolve default namespaces for Queries and Mutations. #394

Closed
wants to merge 1 commit into from
Closed

Resolve default namespaces for Queries and Mutations. #394

wants to merge 1 commit into from

Conversation

robsontenorio
Copy link
Contributor

@robsontenorio robsontenorio commented Oct 16, 2018

Related Issue(s)

Resolves/improves #384

PR Type

Improvement

Changes

This is a improvement about how default namespaces are handled by fields.

Configs in lighthouse.php allow us to define default namespaces for Models, Queries, Mutations ...
But, when using a custom resolver it does not use "default namespaces" configs to resolve the class namespaces.

WITHOUT this PR we must set fullpath for namespace on @group or @field directive. Otherwise a class not found is thrown.

extend type Mutation{
  updateUser(name: String) @field(resolver: "App\\Http\\GraphQL\\Mutations\\UserMutation@update")
}
extend type Mutation @group(namespace:  "App\\Http\\GraphQL\\Mutations"){
  updateUser(name: String) @field(resolver: "UserMutation@update")
}

WITH this PR we can simple do this

extend type Mutation{
  updateUser(name: String) @field(resolver: "UserMutation@update")
}

Breaking changes

There is no breaking changes.

@@ -103,7 +103,12 @@ public function getMethodArgument(string $argumentName): \Closure
throw new DirectiveException("Directive '{$this->name()}' must have an argument '{$argumentName}' with 'ClassName@methodName'");
}

$className = $this->namespaceClassName($argumentParts[0]);
$className = $this->namespaceClassName($argumentParts[0], [
Copy link
Collaborator

Choose a reason for hiding this comment

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

This implementation might cause some really weird issues.

For example, if i have a mutation and a query that are defined in the same way, this will end
up resolving to the mutation class every time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given this schema:

type Query {
  foo: Int field(resolver: "Foo@foo")
}

type Mutation {
  foo: Int field(resolver: "Foo@foo")
}

Now, if you have App\Http\GraphQL\Queries\Foo and App\Http\GraphQL\Mutations\Foo, this will always match the latter.

@robsontenorio
Copy link
Contributor Author

@spawnia what do you mean with "if i have a mutation and a query that are defined in the same way"?

@robsontenorio
Copy link
Contributor Author

robsontenorio commented Oct 16, 2018

@spawnia Ok, i got it.

But do you see what i'm trying to achieve? I am beginner with LightHouse. If you give some directions i can handle it.

@yaquawa
Copy link
Contributor

yaquawa commented Oct 17, 2018

@group is fine for me so far.
Just a piece of code can provide you more clarity.
Also this is the style Laravel follows too.

@robsontenorio
Copy link
Contributor Author

I was trying to keep the schema clean. If there are default namespaces you do not need @group(namespace). But if PR is not compliant with LightHouse, it can be closed by maintainers

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.

3 participants