Break apart Node interface support from node query

JasonTheAdams commented 5 years ago

Describe the issue As came up and was discussed during the development of #871, presently the ASTBuilder::addNodeSupport method checks whether the Node interface is used within types and then adds both the Node interface and node query field.

Thees are really two separate concerns. There are common use-cases where a dev may want the Node interface and not the node query.

Potential Solution I believe it would be better to separate these out and load the node query based on a config option. The Node interface may either continue to be loaded every time or only when used (or the node query field config is set as true).

spawnia commented 5 years ago

We might add a few configuration options relating to relay, maybe like this:


    /*
    |--------------------------------------------------------------------------
    | Relay
    |--------------------------------------------------------------------------
    |
    | Configure options related to supporting Relay.
    | https://relay.dev/docs/en/graphql-server-specification
    |
    */

    'relay' => [
        /*
         * Enable strict validation of Relay server specification compliance.
         */
        'strict' => false,

        /*
         * Expose standardized global object identification https://github.com/facebook/relay/blob/master/website/spec/ObjectIdentification.md
         */
        'objectIdentification' => false,

        /*
         * Default to using cursor based pagination https://github.com/facebook/relay/blob/master/website/spec/Connections.md
         */
        'connections' => false,

        /*
         * Use only single input objects for mutations https://github.com/facebook/relay/blob/master/website/spec/Mutations.md
         */
        'mutations' => false,
    ],

Enabling relay.objectIdentification would add the Node interface and node field.

JasonTheAdams commented 5 years ago

This is a great start! I'm really liking the overall config structure and referencing to the Relay spec.

I've been working through the spec docs and I'm finding that Relay is, in some regards, more flexible than I'd realized. The Edge.cursor for example, doesn't have to be a String, but can be any scalar that resolves to a string. That's difficult to validate.

I'm trying to think through what exactly we can validate, and it's not quite as straightforward as I first thought due to the flexibility of the spec. It's not really even possible to enforce the use of the Node interface as there's not really a good way of knowing which type's should be a node. Validation seemed like a good idea. Hahah!

Otherwise the objectIdentification and connections configurations make sense. For the mutations config is this supported in any way now? And is the clientMutationId currently supported?

My last thought is that I'm really wondering if these things should just be merged into a single relay_mode sort of configuration. I'm trying to think of a use-case where someone would want pieces of the Relay schema. It almost seems like breaking it apart could potentially cause confusing bugs where a dev just forgot to set one of the Relay configs.

Anyway, there's a bunch of thoughts. Curious to hear what folks are thinking!

spawnia commented 5 years ago

I think it does not hurt to allow the user to mix-and-match the different relay options.

For example, one might want the global object identification but not necessarily limit all mutations to a single input argument. The three Relay features are distinct from one another and can be useful on their own.

The relay.strict option would force all three aspects to be enabled and followed thoroughly.

nuwave / lighthouse

Break apart Node interface support from node query #878