findDeprecatedUsages()

[leebyron]

This adds a utility for finding field and enum deprecation usages. The function signature is very similar to validation which should make using it easy where validation is already being used in a reporting flow.

Closes #389

[leebyron]

This utility is a prereq for solving graphql/graphiql#273 correctly

[wincent]

I hadn't seen this before. I presume it is equivalent to '../../index' (might be better to be explicit then?)

[leebyron]

Yeah, node resolution always looks for a index file in a directory. I can break these apart to be more explicit

[wincent]

#ocd

Would be nice to avoid the trailing whitespace when there is no deprecationReason. Maybe:

if (parentType) {
  const reason = fieldDef.deprecationReason;
  errors.push(new GraphQLError(
    `The field ${parentType.name}.${fieldDef.name} is deprecated.` +
    (reason ? ` ${reason}` || ''),
    ...

Or similar.

(And ditto below).

[wincent]

lg2m

[stubailo]

Any reason to not make it actually part of the validation rules? That would make it far easier to include in eslint-plugin-GraphQL, for example, since we let people pass in a list of validation rules to run.

[leebyron]

@stubailo - including a deprecated field is still valid. I don't want to muddy the waters by mixing validation errors and deprecated usages.

I agree we should make it as straightforward to include in the eslint plugin though. I imagine that you would want to have a different configuration for linting deprecated usages, right? Validation issues should be reported as eslint "errors" (2) while deprecation warnings by default should probably be eslint "warnings" (1).

[leebyron]

Something to consider operationally is that any change made on the server that would cause previously valid queries to become invalid (like removing a field in use) are what we consider breaking changes. That means that a safe change to the GraphQL server's schema should not cause previously passing eslint to start failing. However deprecating a field or enum value is considered a safe change - so ideally eslint configs should not be set up to start failing their CI runs if a server deprecates a field.

[stubailo]

I think your reply comes with the assumption that anything in the validation rules folder needs to be included in default validation. I would suggest we can include validation rules in this package that are not turned on by default but just reside in the same folder and use the same API.

For example you might want to make some of the other validation rules into warnings in ESLint as well.

[stubailo]

I think there is a pretty big difference between a linter failing and the code not working anymore - the whole point of a linter is to flag code that would still execute but is undesirable to have in the app.

[leebyron]

I think your reply comes with the assumption that anything in the validation rules folder needs to be included in default validation.

No not necessarily. I wouldn't suggest including this in default validation rules either way. However detecting deprecated uses isn't a validation rule - I don't want to create confusion around that point. This is especially important since graphql.js also serves as a reference implementation and currently everything in the validation rules folder implements a validation rule described in the spec.

We could definitely consider reframing this to have the same signature as a validation rule and provide some additional utilities for assembling and running linters in the same way you would run validation. That would be especially valuable if there are other linter style visitors that we should be building for people. Otherwise this is effectively a linter utility with a single hard coded visitor in it's current implementation.

[stubailo]

Hmm, I think it would fit all of my needs if there was a folder of style rules or similar optional visitors, especially if they used the same API.

[wincent]

I think it's probably fine to merge this as-is, and follow up with subsequent PRs to address extensibility via a folder (or folders) of optional visitors, or other mechanism.

[leebyron]

I think the merits of this utility should still hold even in the context of a more generalized linting interface that works like validation. In the case that we have many things that can be linted, it's likely that you'll still want a way to just extract deprecated uses without running all of lint.

[stubailo]

Yeah I'm not trying to block this PR, just saying that having a "menu" of possible validations available in one folder was super helpful for writing my plugin because not everyone wanted to validate, for example, that all of the fragments are present in the same GraphQL document. The fact that they all had unique names and had the same API let people easily pick and choose just by passing the list of names.

[leebyron]

Yeah - let's head that way for future lint rules. I'm interested in some improvements to the visitor/validator implementation based on learning from good work done on the relay compiler, so that might be an opportune time to make all of this more reusable and powerful