resolving broken deprecation message in docs

[brndnblck]

Relates to:
#157 (comment)

Correcting usages of field.isDeprecated in docs to look at field.deprecationReason instead restores this functionality.

##

CC: @davidcelis @Globegitter

[ghost]

Thank you for your pull request. As you may know, we require contributors to sign our Contributor License Agreement, and we don't seem to have you on file and listed as active anymore. In order for us to review and merge your code, please email cla@fb.com with your details so we can update your status.

[brndnblck]

I've signed the CLA previously, but let me know if you need anything else here.

[gjtorikian]

This doesn't seem like it'll show the deprecation reason though. Without getting too UI y, is it possible to show that inline somehow?

[asiandrummer]

is it possible to show that inline somehow?

It'll probably be something like:

{
  field.deprecationReason && [
    <span className="doc-alert-text">{' (DEPRECATED)'}</span>,
    <MarkdownContent
      className="doc-alert-text"
      markdown={field.deprecationReason}
    />
  ]
}

Instead of reusing doc-alert-text, though, it'd be nice to get a grey-ish text for the deprecation reason.

[asiandrummer]

Thanks for the contribution @brandonblack! I'd love to get the deprecation reason as well if that's not too much troubles for you.

[leebyron]

Hmm, what's happening here?

It's entirely possible that a field is deprecated but also fails to provide a reason.

The previous code looks more correct, so we may need to look elsewhere for the source of the issue.

[brndnblck]

@leebyron looks like we're no longer passing in isDeprecated to the DocExplorer at all. This was working, but broke in the last release. Nothing else references isDeprecated so it wasn't noticed. Everywhere else we're simply interacting with the deprecationReason only.

This patch sorts the issue out, but as you noted is probably just fixing a symptom of the actual issue (eg. we're not passing in both of these field values).

How about I modify this to be field.isDeprecated || field.deprecationReason since that's really what we want anyway?

[leebyron]

I like that solution

[leebyron]

Did it get removed from the introspection query?

[brndnblck]

@leebyron Hmm. Yeah, I really don't know -- definitely not removed.

Digging through commits here and in graphql-js I don't see anything that would have caused this to change recently. It may be that this never worked as advertised.

In any case, I think checking the presence of either of these fields is what we want since deprecationReason is an optional field.

I've resolved this in 0aa9163

[leebyron]

Also, this doesn't have to be done in this PR, but I think we should actually filter out fields into two groups, such that all the deprecated fields appear in their own section after the supported fields. We could have this section collapsed by default if we wish.

[leebyron]

Yeah this is ideal. Hyo and I think we've found the reason behind the confusion and breakage