graphql-js

graphql-js copied to clipboard

I noticed an issue where creating a GraphQLSchema from an SDL and then printing it again results in dropping a @deprecated directive if the reason is explicitly set to null.

In GraphQLSchema we don't store the existence of a deprecated directive and its reason separately. We simply store the deprecatedReason. Because reason has a default, this is mostly fine since even if you don't provide a reason string, you still end up with one. However, the oddities of defaults in GraphQL means that you can pass an explicit null as your reason.

In this case you end up with deprecatedReason: null in your GraphQLSchema which we currently treat as the same as undefined when printing, meaning the @deprecated(reason: null) gets dropped.

I open this PR mostly to raise this issue rather than to advocate for this specific solution. I recognize that interfaces like GraphQLFieldConfig are part of the public API, and thus this might mean the change is technically a breaking change, since there are likely places where people assumed they could set deprecatedReason: null to indicate the field was not deprecated.

Another (even more breaking) option would be to decide that the reason argument to @deprecated should be non-optional, and thus allow us to always assume that every deprecated field has a non-nullable reason.

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	eccf27de6e54ca43135cc406bf3eabf9df4bf90d
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/65978b55cfc25f000706f8ce
😎 Deploy Preview	https://deploy-preview-4006--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

Here's a minimal repro of the issue:

import { printSchema, buildSchema } from "graphql";

const SDL = `
type Query {
  hello: String @deprecated(reason: null)
}`;

console.log(printSchema(buildSchema(SDL)));

/* Outputs:
type Query {
  hello: String
}
*/

My guess is that this is an artifact from the fact that originally explicit nulls were not allowed (or possibly the default superceded them?) -- I am not personally aware of the history, just reading a bit from https://github.com/graphql/graphql-spec/pull/418 -- and that when those changes were made, the directive definition should have been updated from:

directive @deprecated(
  reason: String = "No longer supported"
) on FIELD_DEFINITION | ENUM_VALUE

to

directive @deprecated(
  reason: String! = "No longer supported"
) on FIELD_DEFINITION | ENUM_VALUE

I do wonder then whether a "bug fix" should be issued here in terms of the implementation or in terms of the spec. [Reminds me of https://github.com/graphql/graphql-js/pull/3859]

My guess is that the best course of action is to bring this up at a WG meeting!

Nice find!