Shorthand for type modifiers

[stevelacey]

Argument types with modifiers can get a bit verbose e.g. if you want a non null list of non nulls:

Type::nonNull(Type::listOf(Type::nonNull(GraphQL::type('MyInput'))))

I've written a macro that adds support for variable type syntax GraphQL::parse('[MyInput!]!'):

GraphQL::macro('parse', function (string $string) {
    $modifiers = [];

    while (true) {
        if (Str::endsWith($string, '!')) {
            $string = Str::replaceLast('!', '', $string);
            array_unshift($modifiers, 'nonNull');
        } elseif (preg_match('/^\[.+\]$/', $string)) {
            $string = substr($string, 1, -1);
            array_unshift($modifiers, 'listOf');
        } else {
            $name = $string;
            break;
        }
    }

    if (in_array($name, Type::getStandardTypes())) {
        $type = Type::getStandardTypes()[$name];
    } else {
        $type = GraphQL::type($name);
    }

    foreach ($modifiers as $modifier) {
        $type = Type::$modifier($type);
    }

    return $type;
});

Also adds support for the standard types e.g. GraphQL::parse('[String!]').

The GraphQL::type() method already accepts a string, I'd consider adding in there.

[mfn]

That's some trickery right there.

I don't know enough about the GraphQL DSL, I just hope there isn't more to it the wrapper then wouldn't support, can't say.

I probably won't use it because I made it a happen to use type constant names to reference the types, something like:

Type::nonNull(Type::listOf(Type::nonNull(GraphQL::type(ClientType::TYPE))));

and then I can click on ClientType::TYPE and get there.

With your approach I would start to do string concatenation:

GraphQL::parse('[' . ClientType::TYPE . '!]!')

I value introspection more than fewer characters and unfortunately the readability isn't really given anymore.

Although I'm not disliking the DSL, I always found it difficult when lots of ! are involved, it's hard to see if they're in the correct place or correctly there or not.

Also such an approach (though I understand you just dumped your macro there for PoC) doesn't really parse so if there's a bug in the DSL, outcome but still work or not, who knows, but yield to a wrong DSL what if I miss to close the ] or something.

I.e. looks like tiny new feature but also adds more complexity to maintain, so we've to weight that against the benefits.

PS: and I just realized how much I like the long-winded approach, it's just so more readable. But then, I've been working with it for years so I'm definitely biased 😄

[stevelacey]

@mfn the missing ] problem is catered by the very specific tolerances in the loop, it'll only process the correct forms (both [ and ] together and endsWith !), and anything else hits GraphQL::type and errors as normal, as it should:

>>> GraphQL::parse('String]')
Rebing/GraphQL/Exception/TypeNotFound with message 'Type String] not found.'
>>> GraphQL::parse('!String')
Rebing/GraphQL/Exception/TypeNotFound with message 'Type !String not found.'
>>> GraphQL::parse('[String')
Rebing/GraphQL/Exception/TypeNotFound with message 'Type [String not found.'
>>> GraphQL::parse('[String!')
Rebing/GraphQL/Exception/TypeNotFound with message 'Type [String not found.'
>>> GraphQL::parse('[]')
Rebing/GraphQL/Exception/TypeNotFound with message 'Type [] not found.'
>>> GraphQL::parse('!')
Rebing/GraphQL/Exception/TypeNotFound with message 'Type  not found.'

These errors would be as obvious as any other incorrect type name, I think this is a nice solution in that regard, I gave it some thought to make sure it's reasonable.

As far as I've read listOf and nonNull are the only modifiers in GraphQL, so there's no extra support needed at this time.

Your TYPE const does make this less elegant, I probably wouldn't use this if I were using this approach either, but this is not what is recommended in the documentation, the examples in the README all use strings, and it doesn't bother me that much.

The main reason I want this functionality is because I want to reduce the complexity involved in defining complex type modifier combos, and make it clearer what the resulting end-user queries will have to look like – the project I'm working on has lots of error cases where a user can omit arguments or pass nulls causing resolution errors – a terse DSL for defining nonNull combos makes communicating and spotting incorrect argument definitions a little easier.

--

Also RE your TYPE const, is this a non-standard pattern you've added locally? This doesn't appear to be a native thing – I don't have that const – the best I seem to have is the name attribute (new MyType)->name – some improvements could be made here, also relates to the problem I mentioned elsewhere where the name doesn't have to correlate to the class name or the key name using in the config, all of which conflates the issue, if you've used a different key name in your config (I assume you're not using keys) it'll make your const value mismatch in GraphQL::Type.

[mfn]

Also RE your TYPE const, is this a non-standard pattern you've added locally?

Yes, after about 50 queries and 100 types it started to get annoying to "navigate" the code base and so we just made it a convention:

class AttachmentType extends BaseType
{
    public const TYPE = 'Attachment';

    /** @var string[] */
    protected $attributes = [
        'name' => self::TYPE,
    ];

and then of course the constant TYPE is references wherever it's used; a godsend.

[stevelacey]

@mfn the more I use this macro the more I like it 🤭️ (fyi I am at 120 queries, 80 types) – the ability to quick glance these definitions is valuable, and I don't use ctags or similar anyway 🙊

The verbosity of applying nonNull definitions so far has lead the project I've inherited down a path that has resulted in brittle behaviours – most queries enforce requirement via Laravel rules or resolver logic, or not at all, meaning there's no schema/introspection visibility of those requirements, and in many places, omitting an argument, passing a null, an empty array or an array of nulls creates notices or even fatals.

I think we could do a better job of pushing developers towards favouring GraphQL modifiers over Laravel validation rules for requirement and nullability, and a terser way to define argument types in that way is a step towards that.

If the library requires Type::nonNull(Type::listOf(Type::nonNull(GraphQL::type('MyInput')))); no one will use it, it looks wrong, it makes me think I'm GraphQL'ing wrong, when actually, in most cases, you probably don't want to accept [{}, {}, null, {}] via your API, I expect many consumers are running code in production that crashes when a list of nulls is passed. It was certainly non-obvious to me for a long time that Type::nonNull is a far better way to require an argument than to add a required rule on the field, meaning the user must attempt a request to discover that, and the API I'm working with is full it.

[mfn]

If the library requires Type::nonNull(Type::listOf(Type::nonNull(GraphQL::type('MyInput')))); no one will use it

I always thought it was a basic design mistake they made nullable default 🤷‍♀️

It's the one thing PHP got so much right with it's types, in comparison to e.g. Java 😏

[mfn]

Fixed with #621

[stevelacey]

I always thought it was a basic design mistake they made nullable default 🤷‍♀️

Yeah it's caused me a lot of issues that "optional by default" alone wouldn't have caused given you can supply a defaultValue. Explicit nulls skipping past even the defaultValue has caused me no end of pain. Perhaps there's more we could do there.

I am currently using the pattern of supplying a defaultValue for optional nullable arguments, and then coalescing to that same value within resolves, so that null is handled. Maybe graphql-laravel could support a way to "use defaultValue on null" from the args definition. Maybe nullValue?