Having both isDeprecated and deprecationReason is redundant

[swolchok]

Just define it so that fields are deprecated iff they have a non-null deprecationReason.

[leebyron]

The current reason for this is that deprecationReason is optional, regardless of if a field is deprecated or not, allowing a field to be tersely deprecated and avoiding less good cases like using empty strings to indicate deprecation or something along those lines.

I also felt like it was more clear to have the isDeprecated field, otherwise it seemed easy to misread the list of fields and assume deprecation was always possible.

If we decide to change this though, I would expect that decision to also include making description non-nullable. It's non-nullable for the same reason this semi-redundancy exists.

[leebyron]

Closing this aging issue.

[martinbonnin]

necromancer_mode_on

Can we revive this discussion?

allowing a field to be tersely deprecated and avoiding less good cases like using empty strings to indicate deprecation or something along those lines

Since the current @deprecated directive definition has a default value, feels like terseness is not a problem anymore? One can always deprecate using just the directive:

type Query {
  foo: Int @deprecated
}

We could make reason non-nullable in SDL:

directive @deprecated(
  # note how reason is non-nullable here
  reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION | ENUM_VALUE

This will also forbid such possibly confusing declarations:

type Query {
  # is this deprecated? If yes, why? how is it different than the default "No longer supported"?
  foo: Int @deprecated(reason: null) 
}

Introspection would keep nullable deprecationReason:

type __Field {
  name: String!
  description: String
  args(includeDeprecated: Boolean = false): [__InputValue!]!
  type: __Type!
  # remove (or deprecate 😄 `isDeprecated`)
  # isDeprecated: Boolean!

  # keep the current behaviour for deprecationReason
  "the deprecation reason if the field is deprecated or null if the field is not deprecated"
  deprecationReason: String
}

Looks like graphql-js is also taking that route and deprecating isDeprecated internally: graphql/graphql-js#2700

Thoughts?

[benjie]

@martinbonnin I've re-opened this discussion; but I think the correct way forward would be to add a PR that specifically changes the reason argument to be non-nullable (and no other change).

[martinbonnin]

@benjie PR is here. I'll add it to the next wg agenda.