Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintain comments when parsing schema #2241

Open
gajus opened this issue Oct 27, 2019 · 9 comments · May be fixed by #2471
Open

Maintain comments when parsing schema #2241

gajus opened this issue Oct 27, 2019 · 9 comments · May be fixed by #2471
Assignees

Comments

@gajus
Copy link

gajus commented Oct 27, 2019

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 IvanGoncharov transferred this issue from graphql/graphql-spec Oct 27, 2019
@IvanGoncharov
Copy link
Member

@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
Copy link
Member

@nveenjain
Copy link

@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
Copy link
Member

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

@nveenjain nveenjain linked a pull request Mar 2, 2020 that will close this issue
@IvanGoncharov
Copy link
Member

@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
Copy link

poteto commented Jul 7, 2020

@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
Copy link
Member

@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
Copy link

dchambers commented May 14, 2022

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
Copy link

jimmyn commented Aug 15, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants