Preserve @deprecated when reason = null

[captbaritone]

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.

[netlify]

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

Name	Link
🔨 Latest commit	eccf27d
🔍 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.

[captbaritone]

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
}
*/

[JoviDeCroock]

This looks like a valid change to me as the value is explicitly set to be nullable, and is hinted at at in the description that the reason is usually there

graphql-js/src/type/directives.ts

Lines 224 to 231 in 9c90a23

	args: {
	reason: {
	type: GraphQLString,
	description:
	'Explains why this element was deprecated, usually also including a suggestion for how to access supported similar data. Formatted using the Markdown syntax, as specified by [CommonMark](https://commonmark.org/).',
	defaultValue: DEFAULT_DEPRECATION_REASON,
	},
	},

[yaacovCR]

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 graphql/graphql-spec#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//pull/3859]

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

Nice find!

[benjie]

We should make this argument non-nullable in the spec. Nevertheless, I approve of this change.