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

Add extensibility for easy integration of Apollo Federation PHP (v2) #1040

Open
wants to merge 34 commits into
base: 0.14
Choose a base branch
from

Conversation

Aeliot-Tm
Copy link

@Aeliot-Tm Aeliot-Tm commented Aug 5, 2022

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? yes
Tests pass? yes
Documented? yes
License MIT

The main aim of changes was to make package more flexible for extending by other packages. Now I work on implementation of Apollo Federation PHP (v2) and integration of it into several projects.

I saw updates with the same purposes by @Vincz (https://github.com/vincz/GraphQLBundle). But if understand correct he propose to copy-paste same configuration to the several bundles/packages what is not good on my point of view.

I would be very glad to get review of this PR by community.

BS Breaks:

There is a list of BC breaks. Even property of protected method became nullable it is listed here.

  • Added abstract method: \Overblog\GraphQLBundle\Config\TypeDefinition::getName().
  • Method \Overblog\GraphQLBundle\Config\Parser\GraphQL\ASTConverter\DescriptionNode::toConfig() return empty array when parse GraphQL\Language\AST\TypeExtensionNode.
  • Property $query of the method \Overblog\GraphQLBundle\Definition\Builder\SchemaBuilder::buildSchemaArguments() can be nullable.
  • Refactored class \Overblog\GraphQLBundle\Generator\TypeBuilder(). It is split on several services.

Deprecations

  • Classes \Overblog\GraphQLBundle\Generator\Collection and \Overblog\GraphQLBundle\Generator\TypeGeneratorOptions moved to ..\Model namespace but created stub classes for the backward compatibility which should be removed later.

@Aeliot-Tm Aeliot-Tm force-pushed the add_compatibility_with_apollo_federation_php branch from c51b2d9 to 515f86f Compare August 5, 2022 15:44
@Aeliot-Tm Aeliot-Tm force-pushed the add_compatibility_with_apollo_federation_php branch from c3e8bf1 to cf99c9b Compare August 8, 2022 09:07
@Vincz
Copy link
Collaborator

Vincz commented Aug 8, 2022

Hi @Aeliot-Tm !
Sorry for my late reply, don't have much time.
I'm not sure you really understand what I was working on with the Configuration (this was discussed and validated by @mcg-web at the time). Let me explain a bit more of this work.

  1. The common API
    At the moment, the configuration parsers parse configuration and generate an array of config. This array of configuration is then validated with a Yaml configuration.
    The idea of the new way to do it, is to describe an API. This API describe a schema (all the classes under https://github.com/Vincz/GraphQLBundle/tree/master/src/Configuration).
    The various parsers are considered as ConfigurationProvider. We don't know how they work, we just want them to generate a Configuration.
    For example, the Metadata parser (https://github.com/Vincz/GraphQLBundle/tree/master/configuration/GraphQLConfigurationMetadataBundle) will use annotations and attributes to generate this Configuration object while the YAML parser (https://github.com/Vincz/GraphQLBundle/tree/master/configuration/GraphQLConfigurationYamlBundle) will read a YAML file to generate this configuration.
    So if we want to create a new type of Parser or whatever service that can generate a Configuration object we can.
    We do not duplicate anything. The parsers currently in https://github.com/Vincz/GraphQLBundle/tree/master/configuration will be distinct packages, and we install only the ones we want.
    An other advantage of doing it this way, is the ability to use various type simultaneously (like YAML + Annotations for example) and use type of others parsers. It is not possible with the current version.

  2. Use a service
    With the current version, the parsing is part of the cache warming process. It prevents us from using all Symfony services related stuff. The idea of the new configuration is to manage everything with Symfony services and to generate the Configuration object when it's needed (and cache it). You can check here https://github.com/Vincz/GraphQLBundle/blob/master/src/Resources/config/configuration.yaml for the declaration of the main Configuration object. It is generated by the ConfigurationProvider using all configured providers.

@nicholasruunu
Copy link

This is something we would need. Let me know if I can help.

@Aeliot-Tm
Copy link
Author

@nicholasruunu FYI:
I started work on the new configurations parsing model to help to finish it. And created new issue to collect information about it. Then I'm going to refactor code of this PR to match the new model of config parsing. We can work on it together :)

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.

4 participants