Make the reason argument in @deprecated non-nullable

[martinbonnin]

Follow up from #53 (comment)

Make reason non-nullable:

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

This is technically a breaking change for someone that does this:

type Foo {
  bar: String! @deprecated(reason: null)
}

But feels like this shouldn't be allowed in the first place?

Fixes #53

[netlify]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	665bf71
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/6740c7c5d1172d000830027a
😎 Deploy Preview	https://deploy-preview-1040--graphql-spec-draft.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.

[JoviDeCroock]

Partially related PR in GraphQL JS graphql/graphql-js#4006 where the @deprecated(reason: null) becomes an empty string

[martinbonnin]

Can this be moved to RFC1 or do we need anything else?

Planning to work on a graphql-js implementation for the next wg.

Insufficiently precise review. I meant that I approve of the desire rather than approved the spec edits - oops.

[benjie]

Stage 1 entrance criteria:

• Identified champion - @martinbonnin
• Clear explanation of problem and solution - yes, yes
• Illustrative examples - yes
• Incomplete spec edits - incomplete, yes
• Identification of potential concerns, challenges, and drawbacks - yes, breaking change noted

I'll bump it now.

[benjie]

From the meeting:

• https://spec.graphql.org/draft/#sel-DAJXGTHLAAAEVBAA-7D (and similar locations throughout) < "deprecationReason: optionally provides a reason why this field is deprecated" should be updated - this is not optional now? (I didn't even realise we used "optional" when referring to output values - hence my confusion in yesterday's WG! In fact, these might be the only places we do?)
• If someone does @deprecated without a reason, how should this show up in SDL: with or without the default reason? What if they explicitly set the reason to exactly match the default reason?

Other sections of the spec will need to be addressed too, for example: https://spec.graphql.org/draft/#sel-FAJXLDCAACECx6V should actually have two bangs? Looks like the isDeprecated: Boolean is already incorrect there?

[martinbonnin]

deprecationReason: optionally provides a reason why this field is deprecated

this is not optional now?

I think it has to stay optional nullable for the cases where isDeprecated is false? If not what value do we put there?

I didn't even realise we used "optional" when referring to output values - hence my confusion in yesterday's WG! In fact, these might be the only places we do?

Same! I find using optional for output positions is confusing. Proposal to change the wording to:

deprecationReason: the reason why the field is deprecated or `null` if the field is not deprecated.

We could even go as far as "deprecating isDeprecated" 🙃 :

  isDeprecated: Boolean! @deprecated(reason: "use deprecationReason != null instead")
  deprecationReason: String

[benjie]

it has to stay nullable

100% correct, sorry if I implied otherwise - it was not my intent.

deprecationReason: the reason why the field is deprecated, or null if the field is not deprecated.

I added a comma, but yes looks good to me! (Follow the pattern of the surrounding text if you haven't already.)

We could even go as far as "deprecating isDeprecated" 🙃 :

I think we should leave isDeprecated alone... for now 😉 (But seriously, it's more efficient over the wire than an arbitrarily long string, so we shouldn't deprecate it.)

[martinbonnin]

👍 Makes a lot of sense. And thanks for the comma! Will dive a bit more into this in the upcoming month