graphql / graphql-js

A reference implementation of GraphQL for JavaScript
http://graphql.org/graphql-js/
MIT License
20.06k stars 2.02k forks source link

canonicalize/normalize document utility #2342

Open micimize opened 4 years ago

micimize commented 4 years ago

Feature requests

Would a canonicalize(node) or normalize(node) utility be a desirable feature? I started working on one to simplify some aspects of ast-based code generation, though all it does right now is merge duplicate fields

I don't think it should inline fragments or anything like that (though maybe conditionally). Maybe we'd need to spec out exactly what a canonical document form would actually look like though

IvanGoncharov commented 4 years ago

@micimize Thanks for opening this issue 👍 Sadly we can't merge fields with directives, simplest example would be:

query ($oneCondition: Boolean, $anotherCondition: Boolean) {
  someField @include(if: $oneCondition)
  someField @include(if: $anotherCondition)
}

After merge you will get:

query ($oneCondition: Boolean, $anotherCondition: Boolean) {
  someField @include(if: $oneCondition) @include(if: $anotherCondition)
}

Which is invalid since @include is not a repeatable directive and can have only one instance per location. We can make standard query directives repeatable (skip & include) but it will not solve the same problem with custom directives.

What is your use case for the canonicalize function?

micimize commented 4 years ago

hmm, true. We could just add directives that to the unique condition. My use case is data class generation, but there are other potential use-cases, like getting the correct cache hit on the client for foo { a b } and foo { b a a }, or .graphql file formatting.

As an aside, it feels a bit counter intuitive that

{
    name @include(if: false)
    name @include(if: true)
}

is valid, but not

{ name @include(if: false) @include(if: true) }
IvanGoncharov commented 4 years ago

or .graphql file formatting.

@micimize There is stripIgnoredCharacters function for this: https://github.com/graphql/graphql-js/blob/master/src/utilities/stripIgnoredCharacters.js

I don't think it should inline fragments or anything like that (though maybe conditionally). Maybe we'd need to spec out exactly what a canonical document form would actually look like though

So without inlining fragments, we can only remove fields at the same level that don't have directives applied to them. I don't think there are a lot of real-world queries that have duplicate fields at the same level:

{
  foo
  bar
  foo
}

Can you think of any other normalization step that doesn't affect the query execution?

micimize commented 4 years ago

@IvanGoncharov I was more talking about developer-facing formatting like prettierjs (though this would only be for ASTs).

I guess this makes more sense as a collection of smaller utilities:

This way implementors/users can decide for themselves if things like field order, distinct fragment definition, etc. are part of "query identity", eschewing the issue of whether or not the utilities are fully spec compliant.

To distill my position:

I think the real validation of this idea is if it would be desirable for persisted query implementors to offer some subset of hash(deduplicateFields(sortFields(inlineFragments(node)))) as an alternative to using hash(print(node)) as an identity. @benjamn, you're the most recent committer on apollo-link-persisted-queries - do you know if the apollo team is or might be interested this idea?

Cito commented 4 years ago

A user of GraphQL-core had a similar request. In his case, he wanted to normalize the order of arguments in particular by sorting them. So we are also thinking about something similar to lexicographic_sort_schema, but for executable documents. It's still unclear to me though how much should be normalized. The order of arguments does clearly not matter, but the order of fields may be relevant in some cases. GraphQL follows that order in the result at least if the transport protocol allows it - JSON is officially unordered.

IvanGoncharov commented 4 years ago

@Cito Thanks for excellent example with an order of args. It totally makes sense to have function that does that.