Parse partial source by delegating calls to the internal parseX methods of the Parser

[spawnia]

We use partial parsing extensively within Lighthouse, a schema first GraphQL server for Laravel.

It has been quite cumbersome to implement, e.g. this is what has to be done in order to parse an input value definition:

    public static function inputValueDefinition(string $inputValueDefinition): InputValueDefinitionNode
    {
        return \GraphQL\Language\Parser::parse('
        type Foo {
            field(' . $inputValueDefinition . '): String
        }
          ')->definitions[0]
            ->fields[0]
            ->arguments[0];
    }

With the new API i am proposing, we will be able to just call the Parser directly:

\GraphQL\Language\Parser::inputValueDefinition('foo: String')

I implemented this via the magic __callStatic method on the Parser class to take advantage of the existing private parseX methods.

Because of the way the Parser and the Lexer interact, writing out the individual methods statically would result in a lot of boilerplate, which is why i went for the magic method style. The PHPDoc annotations should provide near equivalent IDE help and static analysis safety.

What do you think of that implementation?

[vladar]

For me, it seems equivalent to just making those parser methods public. Not sure if I am happy exposing those implementation details.

Technically we can do this since those methods will always be a part of the parser (at least as long as there are corresponding AST nodes). It is unlikely to cause any breaks in the future. But still, it significantly increases the API surface comparing to a single unified parse method.

Can you elaborate a bit on the motivation behind this change? Do you have performance/memory issues because of this or is it just inconvenience?

[spawnia]

For me, it seems equivalent to just making those parser methods public.

The API of the parser methods is actually wildly different, they neither take or return any arguments. The __callStatic helper adds the necessary setup: Instantiating the Parser and setting up the expectations for the Lexer.

As long as the AST or GraphQL itself do not change, i think those helpers should be pretty stable. The interface is simple enough: Pass a string and get an AST.

At the moment, i am utilizing the fact that the internal implementation lines up perfectly consistent with what an ideal public API would look like. If internals were to change, we could always rewrite the mapping between the public helpers and the new internals.

Can you elaborate a bit on the motivation behind this change? Do you have performance/memory issues because of this or is it just inconvenience?

I think the schema first approach is a nice way to develop a GraphQL server. We require the AST for some functionality in Lighthouse, mainly server side directives. Sometimes, it is necessary to parse just some bits of SDL, for example when adding to the schema programmatically or allowing some extension points within predefined schematas for the user to fill in.

We have not suffered a large performance/memory penalty, but we do have to maintain our own partial parser, see https://github.com/nuwave/lighthouse/blob/master/src/Schema/AST/PartialParser.php. The example in the initial description honestly seems like a huge hack for something that could be quite simple.

[vladar]

OK, let's go this route. Do you have anything to add in this PR?

I think the schema first approach is a nice way to develop a GraphQL server. We require the AST for some functionality in Lighthouse, mainly server side directives. Sometimes, it is necessary to parse just some bits of SDL, for example when adding to the schema programmatically or allowing some extension points within predefined schematas for the user to fill in.

Just curious are you doing this for convenience/readability (comparing to manual AST building)?

[simPod]

It will need PHPStan ignore error rule to be added for Parser

[mfn]

@spawnia @vladar instead of putting this on top of GraphQL\Language\Parser directly, should this rather be a separate class, so we've cleaner separation between those too? I'm talking about the __callStatic parts and it's phpdoc

Sorry I'm bad with naming, but e.g. GraphQL\Language\ParserHelper or something.

[vladar]

@mfn What if just we make corresponding Parser methods public? @spawnia then you could create a helper class/method like this in your own code.

[vladar]

@simPod Yeah, I forgot that dynamic methods are disabled in PHPStan, thanks for reminding! %)

[spawnia]

instead of putting this on top of GraphQL\Language\Parser directly, should this rather be a separate class, so we've cleaner separation between those too?

The coupling between this seperate class and the Parser would be extremely tight, so i do not see a benefit of splitting them up.

What if just we make corresponding Parser methods public?

Then we would also have to expose the internal functions that deal with the lexer. At that point, we lose any encapsulation and make it impossible to change the implementation of the Parser without breaking users.

then you could create a helper class/method like this in your own code.

That would force anyone else to setup this helper and repeat typing out the 58 parseX method's signatures to get proper autocompletion.

I just added the rest of the helpers, as i still think this solution is the best compromise both in maintainability and usability.

Just curious are you doing this for convenience/readability (comparing to manual AST building)?

That is the main reason, yes. Manually building even the AST is extremely verbose. IDE support for authoring SDL is quite nice as well, so it is quite easy to get it correct as well.

For our particular use case, we really need the SDL as we utilize schema directives as the primary reusable building block. Since they are only a part of the SDL and do not map to executable types, we have to work at this level.

[spawnia]

Btw i renamed a few of the internal methods such as inputFieldsDefinition to inputFieldDefinitions to indicate the plural more clearly and be consistent with other methods such as variableDefinitions.

I do understand the intent behind their original naming though, it made sense in its own way and i am open to reverting this renaming. @vladar this one is up to you.

[spawnia]

@vladar this one is ready to go. I reverted the unnecessary renamings but changed two of the parseX method names to closely match the spec.