GraphQLBundle icon indicating copy to clipboard operation
GraphQLBundle copied to clipboard

Add support for multiple "argsBuilder"

Open Kocal opened this issue 7 years ago • 4 comments

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? .
Version/Branch .

As discussed on a Slack thread, that would be awesome to have the possibility to write something like that:

myField:
  argsBuilders: # argsBuilderS, not argsBuilder
    - 
      builder: MyFirstArgsBuilder
      config: ...
    -
      builder: MySecondArgsBuilder
      config: ...

It would prevent to us to write something not flexible at all:

class MyFirstArgsBuilder implements MappingInterface
{
    public function toMappingDefinition(array $config): array
    {
        $definition = [
            'foo' => [
                'type' => 'Int',
            ],
        ];

		return $definition;
    }
}


class MySecondArgsBuilder implements MappingInterface
{
    public function toMappingDefinition(array $config): array
    {
        $definition = [
            'bar' => [
                'type' => 'Int',
            ],
        ];

        return (new MyFirstArgsBuilder())->toMappingDefinition($config) + $definition;
    }
}

Also, it should handle argument name conflicts. For example, it should throw an exception if MyFirstArgsBuilder and MySecondArgsBuilder both provide foo argument.

Thanks!

Kocal avatar Apr 26 '18 07:04 Kocal

@Kocal What do you expect, when:

  • ~~an args builder emits arg which already defined in configs~~
  • an successive args builder emits arg which already emitted by previous arg(s) builders?

edit Think for the first its useful to behave like the argsBuilder currently does. The question is whether to propagate this logic to args builderS due to maybe confusing situation when you use multiple builders and they mess each other up. This is why with the ability to add types in builder(s) #473 I went the routine of denying overrides. The question is whether it gets confusing when all the builders behave differently in this case. So take this just as a thought for future and ignore the first point :)

akomm avatar Apr 08 '19 15:04 akomm

What do you expect, when an successive args builder emits arg which already emitted by previous arg(s) builders?

Ah you're right, I didn't think about this. I didn't touch GraphQL/ArgsBuilders for months, but I have 2 propositions:

  1. Do a simply array_merge with definitions arrays
  2. Modifiy the toMappingDefinition method to inject a new parameter that is the previous mapping definition, and let the user do what he wants:
class MySecondArgsBuilder implements MappingInterface
{
    public function toMappingDefinition(array $config, array $parentDefinition): array
    {
       // Here, `$parentDefinition` is the result of the previous args builder `MyFirstArgsBuilder`

        $definition = [
            'bar' => [
                'type' => 'Int',
            ],
        ];

        return $parentDefinition + $definition;
    }
}

Kocal avatar Apr 09 '19 06:04 Kocal

I would not yet touch the MappingInterface as it is used in too many different places.

Maybe I phrase the focus of the question differently: Do you think an arg builder replacing args from other arg builder is rather not an error and you want to allow it or rather error you want to know about at compile time after changing the config.

akomm avatar Apr 09 '19 07:04 akomm

... I don't know, both solutions seem good to me ¯\(ツ)

Kocal avatar Apr 09 '19 08:04 Kocal