graphql / graphql-js

A reference implementation of GraphQL for JavaScript
http://graphql.org/graphql-js/
MIT License
20.09k stars 2.03k forks source link

Maintain comments when parsing schema #2241

Open gajus opened 5 years ago

gajus commented 5 years ago
type GlobalCinemaVenue implements Node @preLoad {
  # @todo Refactor into a dynamic set of identifiers, i.e.
  """
  https://webediamoviespro.com identifier.
  """
  foreignWebediaId: String
}

Currently when using parse, the @todo Refactor into a dynamic set of identifiers, i.e. is not made available in AST. As a result, this data is lost when implementing utilities for formatting SDL, e.g. https://github.com/gajus/sort-graphql-sdl.

It would be nice if there was a way to parse/ print SDL while maintaining comments and descriptions.

IvanGoncharov commented 5 years ago

@gajus Thanks for the detailed description, it's definitely something we need to implement 👍 Also, I moved this issue into graphql-js since GraphQL Specification doesn't define AST format.

IvanGoncharov commented 5 years ago

@gajus As a temporary solution you can use the same code as prettier: https://github.com/prettier/prettier/blob/4e46f92b8648c7f25de62bfaf4368aa3f2e588d9/src/language-graphql/parser-graphql.js#L6

nveenjain commented 4 years ago

@IvanGoncharov, Can I work on this issue? We can add support of adding comments separately in AST at the top level... just as done in prettier...

As we perform lexical analysis, we can create a separate comments array whenever a comment is encountered and push the comment in parser._comments...

IvanGoncharov commented 4 years ago

@nveenjain Sounds good 👍 Note: during development please run npm run benchmark to check that you don't introduce performance regression.

IvanGoncharov commented 4 years ago

@poteto Thanks for the interest and help offer. Can you please describe your use case so we can be sure it would be fixed by #2471? Do you need to comments being printed by print or only parsed by parse?

poteto commented 4 years ago

@IvanGoncharov A number of eslint plugins (e.g. eslint-plugin-relay, eslint-plugin-graphql) make use of this parser to parse GraphQL syntax in their lint rules.

One such use case for parsed comments is within GraphQL tagged template literals.

Today, it's not possible for someone to disable a specific eslint (or other linter) rule in the tagged literal. With parsed comments, we could allow that with:

const query = graphql`
  query MyAwesomeQuery($userID: ID!) {
    user(id: $userId) {
      id
      name # eslint-disable-line relay/unused-fields
      profilePicture(size: 50) {
        ...PictureFragment
      }
    }
  }
`;

Obviously, this would not actually be supported in eslint itself. The comment could be prefixed with any arbitrary string. Parsing comments would allow GraphQL lint rule authors to make use of comments in whatever format to skip linting that particular line.

Without parsed comments, the only way today to disable a GraphQL lint rule is by wrapping the entire block with /* eslint-disable */, which is far too coarse grained as it disables the lint rule for the entire tagged literal.

/* eslint-disable relay/unused-fields */
const query = graphql`
  # snip
`;
/* eslint-enable relay/unused-fields */

I've gone back and forth on whether this needs to be addressed in the GraphQL parser or in eslint. I think that it makes the most sense to parse comments here because it's a common feature for parsers and clearly needed for other use cases. I also don't believe this is possible to be reasonably addressed in eslint, because:

  1. The content of a parsed tagged template literal (ie the TemplateElement node) is an unstructured string and therefore un-visitable without a third-party, domain specific parser
  2. A "comment" within the tagged template literal is specific to the domain of the tag and not a concern for eslint
IvanGoncharov commented 4 years ago

@poteto Thanks for the detailed description. It definitely makes sense to handle it in this parser. We want to temporarily limit new features due to TS migration but would definitely work on it as part of 16.x.x line.

dchambers commented 2 years ago

Hi @IvanGoncharov 👋 . We use graphql-eslint but are currently unable to disable linting rules because of tooling we have that uses the parse, visit and print functions from graphql, therefore causing the comments to be stripped from the transformed document. Any update on when this might become possible now that we're in the 16.x range?

jimmyn commented 3 months ago

Is there any update on this issue? Schema stitching is really useful feature, but I'm trying to find a way to keep the comments as we use them to document the API. Is there any workaround?