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

Upstream gqlparser PRs! #1

Open
StevenACoffman opened this issue Mar 18, 2022 · 5 comments
Open

Upstream gqlparser PRs! #1

StevenACoffman opened this issue Mar 18, 2022 · 5 comments

Comments

@StevenACoffman
Copy link

StevenACoffman commented Mar 18, 2022

Hello! I am one of the maintainers of gqlparser and would love to help merge the improvements here into the upstream version. I would really appreciate it if you can open PRs there if you get a chance. I made a mega-giant-one just to remember what all the changes were: vektah/gqlparser#179

People who contributed to that are:
@pawanrawal @JatinDev543 @harshil-goel @anurags92 @NamanJain8

@StevenACoffman
Copy link
Author

@manishrjain Any thoughts about getting back to using the upstream gqlparser? I would rather collaborate than have the community fragmented, but I understand that would take time to reconcile divergence.

@manishrjain
Copy link
Contributor

Hey @StevenACoffman -- I haven't been too close to these changes. If you think the changes can be directly merged, I can raise a PR. @pawanrawal is ideally the guy to do it, but he's now working for a different company.

@StevenACoffman
Copy link
Author

Do either of you know of anyone actively contributing to DGraph who would be able to even review my PRs to replace DGraph's usage with the upstream version?

@manishrjain
Copy link
Contributor

I can have a look for outcaste-io/outserv. I can't speak for dgraph-io/dgraph.

I presume it won't be a breaking change -- everything would work normally?

@StevenACoffman
Copy link
Author

There are some minor inconveniences since the divergence. As I recall there was a minor backwards incompatible change made in the DGraph fork that was never upstreamed. Also, there was one change made to upstream gqlparser to return the error interface type instead of the concrete *gqlerror.Error type.

Originally in gqlparser all methods which were returning an error were using the explicit struct type (e.g. *gqlerror.Error or gqlerror.List). As long as you used the explicit types everything was fine, but as soon as you converted them to the error interface type, nil checks would fail. As a result, gqlparser was changed to return error interface.

So you now need to perform some type assertions to get the fields from gqlerror.Error that are returned as an error:

gqlErr, _ := err.(*gqlerror.Error)

This is how that work was done here:
99designs/gqlgen#2341

I did not make a new v3 release gqlparser because when we upgraded to v2... we found that by following semver (and Semantic Import Versioning) consumers had no way to discover that there was a new version. It ended up causing much more chaos and inconvenience for end users than just staying in perpetual pre-v1.

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

No branches or pull requests

2 participants