-
Notifications
You must be signed in to change notification settings - Fork 112
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
gqlparser v2.3.1 #166
gqlparser v2.3.1 #166
Conversation
Signed-off-by: Steve Coffman <steve@khanacademy.org>
3d36e46
to
c3a58f2
Compare
Thanks for testing and fixing the issue! Is there a reason to upgrade in genqlient? If there's a new feature we need or whatnot I'm happy to but I don't know if there's a reason to make our users upgrade if not (they can always just choose to upgrade in their own go.mod). |
Mostly vektah/gqlparser@v2.3.1 has bugfixes that end users may notice when using current genqlient out-of-the-box. I was looking at how hard it would be to implement #164 by fixing vektah/gqlparser#166 ( some initial work here: vektah/gqlparser#169 ) and I wanted to insure that future releases of vektah/gqlparser would not break genqlient. What's Changed in gqlparser v2.3.1 since v2.2.0
|
I feel ben should have the final word, but I'm fine with this fwiw. |
Thanks for the explanation. I thought about it more and you've convinced me -- since gqlparser is internal to genqlient it makes good sense for us to upgrade it even though users may also use it via other libraries (i.e. gqlgen). If it doesn't work right they see that as our bug even if the cause is in gqlparser. |
gqlparser v2.3.1 appears to work ok, but v2.3.0 gqlparser had a PR that needed to be reverted.
FYI I cut a new v2.3.0 gqlparser release https://github.com/vektah/gqlparser/ thinking it had been a really long time, but it looks like the vektah/gqlparser#112 might have been half baked as it causes problems for gqlgen and genqlient:
Signed-off-by: Steve Coffman steve@khanacademy.org