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

Consider making MISSING_OPERATION_NAME_CODE optional. #346

Closed
collin opened this issue Jul 29, 2024 · 4 comments · Fixed by #362
Closed

Consider making MISSING_OPERATION_NAME_CODE optional. #346

collin opened this issue Jul 29, 2024 · 4 comments · Fixed by #362

Comments

@collin
Copy link

collin commented Jul 29, 2024

I've found issues around this and understand this project considers it best practice to include an operation name.

I am however, several dozen graphql queries in to a project that does not name operations and while other features of the LSP/tada are great (phenomenal actually, thank you!) the warnings are intrusive/something I'd rather not approach at the moment.

Is there any chance that stance could be reconsidered to make named operations optional rather than automatically warn?

@kitten
Copy link
Member

kitten commented Jul 29, 2024

In what context are you using GraphQLSP? If it's in the context of gql.tada, then we could consider lowering it to an info level, which should make it less intrusive, i.e. depending on your editor and settings it's basically going to show up as a hint.

However, I believe, if you're using it together with graphql-code-generator, I vaguely remember there was a requirement for it, due to type generation.

Edit: On a side-note, since I'm curious, realistically, what is the hesitancy and problems you see with naming the queries?

I've migrated some legacy libraries in codebases before, and naming ~100 things isn't the first scary thing that'd come to mind. In GraphQL, naming operations is certainly not something I'd want to spend a lot of time on, but if you've got a query per screen via fragment composition (or at least a query per component) it doesn't feel like a blocker per se. I'm basically trying to gauge what level of problem this is to you ✌️ (i.e. blocker / annoyance / technical hurdle / complex requirements, etc)

@collin
Copy link
Author

collin commented Aug 1, 2024

I am using this with gql.tada. Not through graphql-code-generator

The way I use graphql is via a hasura service. I'm using graphql on my back-end services in order to hook into hasura's permissions system.

The "intrusiveness" is mostly to do with the warnings showing up in the editor:

Screenshot 2024-08-01 at 12 15 20

I'd rank mostly as an annoyance.

@kitten
Copy link
Member

kitten commented Aug 1, 2024

oh, I wasn't aware that we highlight the entire document. That should definitely be adjusted as well. I don't think we want to really mark the entire document in this case 🤔

@JoviDeCroock
Copy link
Member

That's indeed true, the reason for this were historical. When we initially created GraphQLSP it was with tagged-template literals which when uesd often were just the following

gql`your-document`

Then the code-generator would run and generate types, which would be imported from a co-located file. It would often be confusing when nothing was generated which is the main reason behind flagging the whole document. Generally it would be better for the call-expression equivalent to do this either on the variable-declaration or drop this warning entirely, I do think from an observability perspective this warning is great.

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

Successfully merging a pull request may close this issue.

3 participants