softonic / graphql-client

PHP Client for GraphQL
Other
123 stars 23 forks source link

Ability to change uri for query #27

Closed 4n70w4 closed 4 years ago

rccrdpccl commented 5 years ago

Thanks for the contribution @4n70w4 .

Could you please explain your use case behind this, so that we could better understand?

Could you also please add a simple test case?

4n70w4 commented 5 years ago

@rccrdpccl example:

I reuse the already configured guzzle client and needs to define the endpoint for the request.

About the tests, this does not affect your library. Also does not create new mutants in mutation testing.

rccrdpccl commented 5 years ago

Isn't this configurable by passing guzzle options to the client? I am curious to understand in what case you'd need to redefine request uri from already initialized client.

Tests are not only for the code that is already there, but it prevents future changes to break the functionality that you are introducing. However we can wait to define exact use case before writing tests.

4n70w4 commented 5 years ago

use Softonic\GraphQL\Client;
use Softonic\GraphQL\Response;
use Softonic\GraphQL\ResponseBuilder;

....

    public function something(array variables, $query, $uri) : Response {
        $client = new Client($this->guzzle, new ResponseBuilder() );

        return $client->query($query, $variables, $uri);
    }

....

// another methods to use guzzle client for non graphql requests
rccrdpccl commented 5 years ago

Thanks for the example. What is the reason for re-using guzzle client?

joskfg commented 5 years ago

Thanks @4n70w4 for your PR!

I think add the uri in the query call could be not the best option. We could find a better solution if we know the exact use case to do something cleaner if it is possible.

If the case is to access two different graphql servers I think that the best option is to create two clients. This make sense because probably the clients should have different options. For example, the authentication could be different.

4n70w4 commented 5 years ago

In your opinion hardcode uri as a blank line is the best option?

4n70w4 commented 5 years ago

@rccrdpccl I already have a configured client (authorization, certificates, proxies, etc.) for access to the provider’s endpoints. I see no reason to configure another client with a difference only in the url '/ graphql'.

rccrdpccl commented 5 years ago

@4n70w4 Ok, thanks for explaining, it seems a reasonable use case. I think your approach it's a good improvement for the library, ideally we should end up with something like public function query(string $query, array $variables = null, string $uri = '', array $options = []): Response, but for now we can add parameter $uri and leave options to be added in the near future.

I will add a few small comments to the PR so we can get it merged as soon as possible!

In order to solve this long term, we are planning to release v2, where we will need to configure the client in a similar fashion: new Client($this->guzzle, new ResponseBuilder(), '/quert_endpoint', '/mutation_endpoint' );. Would this work for your use case?

4n70w4 commented 5 years ago

I already use my fork in production. I don’t know if it will be time to go back to this PR.

Such a design could also solve similar problems:

new Client($this->guzzle, new ResponseBuilder(), '/endpoint');
rccrdpccl commented 4 years ago

Close for inactivity. If interested in implementing this issue, please re-open it or file an issue