-
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
Preserve directives on fragments when merging AST #3290
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: eeffefb 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 |
242b36d
to
a72087b
Compare
|
awesome @esquevin, thank you ! one thing to note about signing the CLA, the email in your commits and the email you sign the CLA with have to be the same |
I've signed the CLA, using the proper email On another note there seems to have been an issue in the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3290 +/- ##
==========================================
+ Coverage 55.34% 55.36% +0.01%
==========================================
Files 115 115
Lines 5348 5348
Branches 1450 1450
==========================================
+ Hits 2960 2961 +1
Misses 1962 1962
+ Partials 426 425 -1
|
packages/graphiql-toolkit/src/graphql-helpers/__tests__/merge-ast.spec.ts
Outdated
Show resolved
Hide resolved
packages/graphiql-toolkit/src/graphql-helpers/__tests__/merge-ast.spec.ts
Outdated
Show resolved
Hide resolved
packages/graphiql-toolkit/src/graphql-helpers/__tests__/merge-ast.spec.ts
Outdated
Show resolved
Hide resolved
@esquevin this one is almost ready! you just need to run |
589f819
to
948c569
Compare
|
b8886a9
to
fe0201b
Compare
@acao Seems everything is green, can I do anything to help this get merged? |
@esquevin just needs a rebase because of conflicts from the PR of yours we just merged! |
ea92079
to
55a30e8
Compare
Co-authored-by: Dimitri POSTOLOV <[email protected]>
55a30e8
to
f571341
Compare
Everything should be good now =) |
@acao I believe this can be merged, let me know if it ain't the case and what I can do. I thought it had been merged a while ago 😅 just realised now that it never made it across the finish ine |
When a FragmentSpread has a directive, they used to be discarded.
Added a test and a fix. All tests passes