consider not deprecating typeInfo and getFieldDef options to validate function and TypeInfo constructor

[yaacovCR]

trafficstars

https://github.com/graphql/graphql-js/blob/e1726dfea66979bfe7ad1c0b0834613e4b6ce4b4/src/validation/validate.ts#L44-L45 https://github.com/graphql/graphql-js/blob/e1726dfea66979bfe7ad1c0b0834613e4b6ce4b4/src/utilities/TypeInfo.ts#L66-L67

From what I understand, by preserving these options, we add the ability for a server to add custom meta fields such as __fulfilled or __directive and still validate properly.

[yaacovCR]

https://github.com/graphql/graphql-js/blob/e1726dfea66979bfe7ad1c0b0834613e4b6ce4b4/src/utilities/TypeInfo.ts#L311-L322

The default function here is what explicitly allows the meta fields included within the spec. Adding a custom version of this function would allow for custom behavior.

[yaacovCR]

Alternatively, we could allow passing a list of pairs/tuples of check functions and meta fields instead of an entire custom function.

[yaacovCR]

Related: https://github.com/yaacovCR/graphql-executor/commit/00f54c0df07ff6317c640077669a339da250fe35 And: https://github.com/yaacovCR/graphql-executor/pull/170

[yaacovCR]

@IvanGoncharov i believe #3575 completes some of the above planned deprecation

While expected, I think that’s unfortunate, for the reasons outlined above.

Could you comment here when you get a chance? Maybe there are alternatives I have not considered?

[IvanGoncharov]

@yaacovCR I'm working on moving this function into GraphQLSchema. It would allow you to subclass GraphQLSchema` and override this function. Would this work for you?

[yaacovCR]

Maybe, but why is that better? It would mean schemas have to be customized to add custom meta fields which by definition exist outside the schema and could be executor specific.

Could this also be restored until there is a replacement?

[yaacovCR]

Maybe if there is not consensus we can discuss further at next js wg?

[saihaj]

The PRs that are made there is little to no context on why things are deprecated. PRs have no description for why we are making changes. We should be documenting all the changes we make so it is important that we first create an issue for broader discussion and then use that as an item in the JS WG.

[yaacovCR]

Some thoughts on this issue.

1. In theory, I would imagine that whatever solution we have for introspection types should match the solution for the default directives. This would argue for them both being included within the GraphQLSchema JS object, or both excluded.
2. In practice, the spec says that all built-in directives are to be returned during introspection, but meta-fields like introspection root fields should not. The current implementation super-"matches" the spec in that introspection types are not just hidden from the schema on introspection, they are not included at all.
3. I imagine that you are planning on changing point 2 so that the introspection fields are included within the root query object type, but hidden from introspection, as they are meta fields. This would still kind of match the spec. The spec is kind of silent on whether the introspection fields are actually part of the Query type and hidden, or not part of the root Query type.
4. The above would work for __type and __schema but not really for the __typename metafield, unless that field is also added by default to every object type (except subscription objects?) and then hidden on introspection. I guess you would be planning to do that also? I'm not sure how that would work for subscription object types, as they are regular object types that happen to be specified within the schema as for the subscription type. perhaps you are also going to introduce a new GraphQLObjectType subclass, something like GraphQLSubscriptionObjectType -- I think you have pointd out already that it doesn't make sense to have resolve and subscribe config and properties under GraphQLObjectType, as it is either/or depending whether we are talking about a SubscriptionType. Maybe you will take the opportunity to fix that as well?
5. Does it really make sense to add a metafield to every type as opposed to the current check against the type name outside of the type system?
6. In any case, assuming you do add the __typename meta field everywhere, then all the fields and metafields are in the schema so the function at https://github.com/graphql/graphql-js/blob/f44cb4e7983ae8daf5cdccd3db26380e0a8f19eb/src/execution/execute.ts#L1028-L1049 reduces to the single line https://github.com/graphql/graphql-js/blob/f44cb4e7983ae8daf5cdccd3db26380e0a8f19eb/src/execution/execute.ts#L1048
7. And a custom executor could still customize execution by using a custom getFieldDef function.
8. The problem for custom executors is that validation is not so easy to customize after you deprecated the custom TypeInfo and custom getFieldDefFn options.
9. Total aside, I noticed just now you forgot to delete this comment https://github.com/graphql/graphql-js/blob/f44cb4e7983ae8daf5cdccd3db26380e0a8f19eb/src/validation/validate.ts#L35-L36
10. If you introduce ability to subclass the GraphQLSchema option, you could add the custom introspection types and meta fields there. Would the same thing be for customizing directives? Is that better than using custom specifiedDirectives and specifiedTypes arguments to the regular constructor?
11. Both options would mean that to customize execution, you have to recreate the schema, which is a small performance hit?
12. Maybe we would also introduce the ability to combine multiple schema parts within the spec? Which would be huge and maybe beneficial?
13. The above roadmap is kind of involved, and I think we should just restore the deprecated bits in #3575 and #3574 until we figure it out.

[IvanGoncharov]

The PRs that are made there is little to no context on why things are deprecated. PRs have no description for why we are making changes. We should be documenting all the changes we make so it is important that we first create an issue for broader discussion and then use that as an item in the JS WG.

@saihaj It has all the necessary context in PR, see yourself: https://github.com/graphql/graphql-js/commit/825ebd312269decd0ac3872a3346d85d5cb6e262#diff-17896bb86b8ded29df8171b14466be9815990d607597e788cf166fb7f56c519dL25-L27

This option was added to support FB-specific pre-specification stuff and these messages were there for years. I disagree that the removal of explicitly marked hacks should be discussed beforehand otherwise every change will take ages to do.

Stuff gets marked as @deprecated and stays deprecated for the entire release, a bunch of tooling supports this directive so the community has plenty time to react. And this issue is a good example, I just missed it 😞 otherwise I would discuss it here beforehand.

Moreover, nothing gets released without an extensive RC phase. I think 16.0.0 was an example of a really productive RC phase where a bunch of stuff was improved, fixed, or even reverted before public release.

So no harm is done since we are not even in the RC phase, and have plenty of time to discuss what to do including reverting stuff.

[IvanGoncharov]

I imagine that you are planning on changing point 2 so that the introspection fields are included within the root query object type, but hidden from introspection, as they are meta fields. This would still kind of match the spec. The spec is kind of silent on whether the introspection fields are actually part of the Query type and hidden, or not part of the root Query type.

@yaacovCR Sorry for confusing you. My idea was to add just to add getFieldDef to the GraphQLSchema as such:

class GraphQLSchema {
  // ...

  getFieldDefinition(parentType: GraphQLCompositeType, fieldName: string) {
    // ...
  }
}

So if you want to change it you can do:

class GraphQLShemaWithAdditionalMetaFields extends GraphQLSchema {
  // ...

  getFieldDefinition(parentType: GraphQLCompositeType, fieldName: string) {
    if (fieldName === '__fulfilled') {
      return; // ...
    } 
    return super.getFieldDefinition(parentType, fieldName);
  }
}

[IvanGoncharov]

@yaacovCR That way you change how fields are resolved for a particular schema, not for a particular function. So it enforces interoperability across all ecosystems of libraries. Also, it's comparable to how Parser and all other parts of 'graphql-js' are extended.

[yaacovCR]

I agree we should standardize but don’t think this is how we should do it… I think this example shows how interoperability is actually hampered with this approach. Instead I think we should add a getFieldDef option to execute.

[yaacovCR]

As above, I also think #3575 and #3574 should be reverted until this is worked out.

[yaacovCR]

Put more succinctly, graphql-executor is designed as a drop in replacement for execution with graphql-js and this breaks that. We should be moving imo in the direction of moving all Execution logic out of schema creation rather than the reverse.

I don't oppose creation of a default schema getFieldDef method that the custom ones provided to validate and execute can call, but I think it's important that we preserve customizability, especially now that envelop provides well known ways this can be modularized/standardized.

@dotansimha

[yaacovCR]

You could counter that adding a metafield is a schema concern and not an execution concern, because metafields implicate validation and execution.

I still think I would handle this differently; I think we should provide a way to combine multiple schemas or validate/execute against multiple schemas, ie one that has user types and another that has execution directives/metafields. This would be a generic solution to multiple problems, ie adding defer/stream.

Even with your approach, I would set up differently, with a config option for schema creation, and I would definitely standardize how specified/custom metafields and specified/custom directives are handled.

I apologize for not raising this in the last wg, btw! I filed this issue a while ago and then forgot about it. I remembered only when I saw #3575 and #3574. My review on those PRs would have been to hold off until we can settle on a better approach.

Questions for js-wg:

1. Are meta-fields a schema or execution concern-- where should they live?
2. Should we introduce a way to combine or validate/execute against multiple schema parts to solve addition of execution-specific schema parts?
3. Should customization of behavior occur in method overrides or in configuration?

[yaacovCR]

As soon as #85 is merged, I will add an agenda item to discuss this further at the next WG.

In advance of the meeting -- as an immediate action item -- attn: @IvanGoncharov -- I again suggest that #3574 and #3574 should be reverted.

[IvanGoncharov]

As agreed I reverted both PRs until we discussed it on a WG.

As soon as https://github.com/graphql/graphql-js/pull/85 is merged, I will add an agenda item to discuss this further at the next WG.

Please go ahead, I merged https://github.com/graphql/graphql-js-wg/pull/85 I suggest having two separate discussions:

1. Should we allow 3rd-party (not part of the official RFC process) non-spec compliant customizations? Disclaimer: I'm fully against it.
2. What to do with getFieldDef?

I think we need a policy on question 1 since we already had a similar discussion about adding hooks to the parser, executor, etc. Otherwise, we will have this type of disagreement every time someone wants to add or remove a legacy hook (we still have a few of them).

[yaacovCR]

Thanks for addressing this. In general, a hook should not be removed in my opinion until a satisfactory replacement method for customization is introduced.

Although I disagree with subclassing GraphQLSchema and overriding the getFieldDef schema because it means that schema recreation is required to add nonspec metafields, that functionality in any event has not yet been introduced. Moreover, it is not satisfactory.

An additional alternative would be to create a Validator class that can override validation, but you would still need the extra argument for TypeInfo. These hooks are therefore different than the hooks we discussed previously.

See also the alternatives I already raised above. They are more involved.

Overall, I think the current hooks are the simplest and best. They allow non-spec behavior, but they don't mandate it, obviously, and I don't think we should rely on cumbersome hooks rather than efficient ones just because graphql-js has the dual role.

Looking forward to a spirited discussion on this at the next js-wg. ;)

[IvanGoncharov]

Thanks for addressing this. In general, a hook should not be removed in my opinion until a satisfactory replacement method for customization is introduced.

@yaacovCR This hook was clearly marked as "experimental" and later "deprecated". Moreover, before deprecation, it said "You should never need to use it.". Please see: https://github.com/graphql/graphql-js/commit/825ebd312269decd0ac3872a3346d85d5cb6e262#

So no replacements were promised. If this feature had a use case for "spec compliant" implementation it made sense to provide some alternative but from what I see it doesn't. Moreover, I reconsider my position and think that by providing "compromise functionality" we just shifting this discussion down the road.

So instead of mixing two discussions, I want to solve the core question we have at hand: Is supporting non-compliant GraphQL implementations a valid use case for graphql-js? I added a separate agenda item to discuss just it. I don't think discussing the technicalities of getFieldDefFn makes sense before we have the answer to that question.

[yaacovCR]

I don’t think it makes sense to discuss a broad policy about custom hooks without reference to the features that the hooks provide and the alternatives that are available.

I am afraid I am not really interested in that policy discussion in a vacuum — your position seems clear, and if we are not considering the trade offs, I am afraid I have little to offer to convince you otherwise.

i think the discussion would be much more fruitful from the other direction. Let’s just agree that the existing hooks are not ideal — if only even because you don’t like them — and think together about other possible options. What do you suggest? Is there an alternative that does not require a costly additional round of GraphQLSchema construction?

[IvanGoncharov]

Let’s just agree that the existing hooks are not ideal — if only even because you don’t like them — and think together about other possible options.

This is the issue I don't like to be a "bad guy" who removes "features" or blocks PRs because he doesn't like something. I voiced my opinion in multiple discussions saying "we should allow non-spec compliant behavior through public APIs". We had this discussion multiple times on different parts of graphql-js. And now we are back to the same discussion again.

It's exhausting for we to have had this argument every time and moreover, I don't want it to be treated as "Ivan doesn't like this API". Either we agree on "graphql-js" supporting only spec-compliant GraphQL APIs or not. And I'm ok if consensus goes either direction, I just don't want to have this conversation all over again.

The point of not adding complexity just to support "non-spec compliant" GraphQL APIs is the only reason I like to remove getFieldFn, otherwise, I think it's a perfectly fine API.

If we agree on that policy, it means no public or stable API can be provided. And no functionality should be designed exclusively to support "non-spec compliant" GraphQL APIs.

That said, I also feel strongly about not doing anything that actively prevents such use cases. For example, all internal exports are accessible (even in ESM), all private fields are just marked with underscore without making true JS private fields, and nothing prevents you from subclassing stuff and overriding certain functions, I'm totally ok with exporting anything as "unstable" API, etc.

What do you suggest? Is there an alternative that does not require a costly additional round of GraphQLSchema construction?

This API was added with a clear message:

    // NOTE: this experimental optional second parameter is only needed in order
    // to support non-spec-compliant code bases. You should never need to use it.
    // It may disappear in the future.

I made the mistake of replacing it with a short deprecation message because I assumed it would be enough. So I think removing it completely is totally reasonable.

But I suggested adding getFieldDef to the schema since it is valuable on because it allows to not only share this functionality between executor and validator but also to prevent other projects from copy-pasting it: https://github.com/apollographql/apollo-ios/blob/7afcfc3c32950f7f6b6d768212f12412701ec7d6/Sources/ApolloCodegenLib/Frontend/JavaScript/src/utilities/graphql.ts#L94

Same with the Parser class, I didn't convert to class just to allow customization of the parser. I did this conversion to remove passing "parsing options" between every function. Exporting it as an "unstable" API is just a byproduct of this conversion that adds almost no cost (comment + export).

So I'm not ok designing new functionality from scratch to executive solve a use case that I feel is misaligned with graphql-js goals. Note: this goal is up to discussion that's why it's on the WG agenda.

If you can suggest a use case (beyond supporting non-spec compliant GraphQL) or refactoring that makes sense on its own, but also enables non-spec compliant API (with no special guarantees for that use case) I'm ok with it. Adding getFieldDef to the schema is the only solution I can think of, that provides a stable API (since changing its signature is a breaking change).

[yaacovCR]

As an aside, your cite to Apollo code made me remember that the function differs between validate and execute because in validate the parent type is a composite type and unions are treated differently. That may slightly complicate your efforts here to fold the function into the class; I guess you would need two methods that share some code.

[yaacovCR]

The point of not adding complexity just to support "non-spec compliant" GraphQL APIs is the only reason I like to remove getFieldFn, otherwise, I think it's a perfectly fine API.

I suppose the meeting may then be valuable after all. Although I have nothing to add, and your position is plain, we may be able to gather additional voices that may help guide us toward the commonly held sentiment. From my experience at these meetings, however, I would be surprised if many additional parties attend and even if a handful do attend, i don’t believe it will be a representative sample of the stakeholders. This policy question might gain more traction at the main graphql-wg although it does not hurt to try at the js-wg first. Looking forward to understanding others’ views.

[yaacovCR]

If my view wasn’t clear enough already, I think graphql-js should provide a production caliber graphql execution pipeline that can also serve as the reference implementation.

[Think: dual mandate (cf. US Federal Reserve mandate with respect to price stability and full employment)]

I believe the above goals may sometimes be in tension but that collaborative work and diligence by maintainers will find a happy balance. Things that probably can’t be included: jit compilation (unless it can be abstracted to a language independent algorithm?) things that easily can: custom hooks that allow experimental metafields.

[IvanGoncharov]

If my view wasn’t clear enough already, I think graphql-js should provide a production caliber graphql execution pipeline that can also serve as the reference implementation.

@yaacovCR I think in an opposite way: graphql-js gathers production caliber practices from JS and other communities to drive spec changes (that's why all spec changes should have graphql-js PR) and promote it across the ecosystem by being a reference implementation.

Things that probably can’t be included: jit compilation (unless it can be abstracted to a language independent algorithm?) things that easily can: custom hooks that allow experimental metafields.

Actually it's the opposite, I'm skeptical of JIT makes sense in all languages and also I don't think it is worth adding that complexity (we need still to maintain a standard executor, since not every language has eval) but I can't rule it out immediately the same way as I do with getFieldDefFn.

JIT doesn't break spec so there is no hard no from me. It will require a lot of JS-specific code but potentially it replicable in other languages (except that only dynamic languages have eval). So in short I personally have some opposition to including a JIT-based executor but it's up to discussion and weighing pros/cons.

On the other hand, allowing for hooks that bypass spec replicates to all other languages just on a basis it's in graphql-js: see it in Python, see it in Go, see it in Perl, see it in Lua. And those are results of quick Googling by arg name, high change that other implementation also has it or will have in the future. Also please note that this happened even though this feature is marked both as experimental and "You should never need to use it.". And just imagine how many implementations would have it without those "scary" labels. For most implementations, if something gets merged in graphql-js it gets immediate green light on being merged to GraphQL libraries. So if we add something to graphql-js we don't just decide what is good for the JS ecosystem we automatically promote the same decision in all other languages.

So in a lot of senses graphql-js influence GraphQL interoperability more than the spec ever will, not only through language ports but also through compatibility with tools like GraphiQL, prettier, etc. The absolute majority of GraphQL users don't read spec (and that is a good sign) so they care more about the server working with those tools than they will ever care about it being "spec compliant".

I will never prevent anyone from using stuff from graphql-js and monkey-patching it to do something that doesn't adhere to the spec. But I'm against making it "officially supported" and motivating implementations in other languages to also do that.

[yaacovCR]

For the same reason that I think these hooks are fine for graphql-js, I think they are fine for the rest of the ecosystem.

[yaacovCR]

The hooks are language agnostic and therefore work fine within the reference implementation.

If jit compilation can be structured in some higher level abstraction that is language agnostic, it could also be included, but that would be a heavy lift.