-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Support complex queries #3298
Support complex queries #3298
Conversation
🦋 Changeset detectedLatest commit: b62ed38 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
7c51edc
to
da51fb4
Compare
|
176a1e7
to
534f506
Compare
@esquevin thanks for this! dumping huge files into the test fixtures that break the functionality is more than welcome. If our tooling can't handle complex use cases then it's not serving it's purpose. A few years back, I proposed to move the merge ast method into graphql-language-service utils because it would make more sense, and would be useful for an LSP codelens as well. There are more utils there that use graphql AST visitors to give you some testing patterns to work with. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3298 +/- ##
==========================================
+ Coverage 55.69% 55.76% +0.06%
==========================================
Files 110 110
Lines 5237 5242 +5
Branches 1425 1425
==========================================
+ Hits 2917 2923 +6
Misses 1896 1896
+ Partials 424 423 -1
|
d6ec689
to
8dc72ee
Compare
Added the test showcasing the error. If you checkout df8d7a1 and run the test you should get:
|
|
cea53fd
to
858250c
Compare
this is awesome, thanks for this! it seems ready to go! |
|
3d1a4e9
to
41ce863
Compare
41ce863
to
12a1282
Compare
@acao Seems everything is green, can I do anything to help this get merged? |
Use a 2 pass strategy
12a1282
to
b62ed38
Compare
@esquevin thanks for everything! nothing additional needed, there is just a lot in flight, and bugs take precedence, and I'm busy with interviews, so I'm hoping to release it either today or sometime in the next few days |
✅ Deploy Preview for graphiql-test ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This MR address an error that breaks the merge fragment feature. I think mutating the selection while iterating on it creates some structures that are way too deep and/or cyclical.
I stumbled upon the issue on https://api.sorare.com/sports/graphql/playground while trying to merge the fragments of this query:
Issue only seems to happen when merging the query with the schema provided.
I spent some time poking at the code without success until I had a hunch and split the visitor logic into a 2 pass process which fixed the error.
I'm concerned about how best to test it as I wasn't sure I could dump a huge schema and and a huge query in the repo for one test. What would you advise?