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

feat: parse string documents to extract the operationName #325

Conversation

tsyirvo
Copy link
Contributor

@tsyirvo tsyirvo commented Feb 11, 2022

Following a previous PR from another contributor that allowed getting the OperationName from a GQL operation, I added the possibility to also get it when the document is a string.

Currently, operationName could only be extracted from "regular" queries/mutations, and it wouldn't work for operations passed as strings.

In our current project, all our queries and mutations are auto-generated, and they all are strings, hence the operationName was never resolved and passed.

I just used the parser exposed by graphql to get the actual operation from the string, then used the existing logic for getting and setting the operationName if found.

@jasonkuhrt

This comment was marked as outdated.

@tsyirvo
Copy link
Contributor Author

tsyirvo commented Feb 14, 2022

Since print from graphql/language/printer was already used, I thought there wouldn't be much change by adding the parse method from graphql/language/parser.

The package seems to be already used for other things than static typing now, or am I mistaking?

@jasonkuhrt
Copy link
Owner

@tsyirvo oh interesting 20f18d3 ... Ok.

Then disregard my comment. I'd rather roll forward on this than back. We can change our policy with regard to the graphql dep. It is not crazy anyways that it would be required as a peer dep or used internally. But will need some thinking perhaps to find the optimal way forward (rather than this current accident).

Meanwhile we can move forward with this.

Copy link
Owner

@jasonkuhrt jasonkuhrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsyirvo please add tests and if you can adjust the docs that would be great.

@tsyirvo
Copy link
Contributor Author

tsyirvo commented Feb 14, 2022

Thank you for your feedback.

I updated the PR, let me know if I need to do something else.

Copy link
Owner

@jasonkuhrt jasonkuhrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jasonkuhrt jasonkuhrt merged commit 26711e7 into jasonkuhrt:master Feb 14, 2022
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 this pull request may close these issues.

2 participants