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

upgrade to graphql v17 including support for new incremental delivery format #3682

Merged
merged 9 commits into from
Aug 13, 2024

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Aug 7, 2024

This PR provides support for the next iteration of the incremental delivery response format. It is based on the proposed changes to @graphql-tools/utils, that library being the inspiration for the most recent changes within this repository in #3580.

I am not sure what the testing story will be for this PR as I am not too familiar with development with this repository. Feel free to point me in the right direction or to take this PR over. :)

Copy link

changeset-bot bot commented Aug 7, 2024

🦋 Changeset detected

Latest commit: 5d0470c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
graphql-language-service-server Minor
@graphiql/plugin-code-exporter Major
graphql-language-service-cli Minor
@graphiql/plugin-explorer Major
graphql-language-service Minor
vscode-graphql-execution Minor
codemirror-graphql Minor
@graphiql/toolkit Minor
@graphiql/react Minor
monaco-graphql Minor
vscode-graphql Minor
cm6-graphql Minor
graphiql Minor

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

@yaacovCR yaacovCR force-pushed the new-incremental-format branch 3 times, most recently from 91eb569 to 02f62a2 Compare August 12, 2024 15:26
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Aug 12, 2024

I have enabled end to end tests and added v17 to the testing matrix. Unfortunately, there are some build errors that seem to be possibly on Main or possibly some sort of local development error on my side.

It also appears that there are some new test errors with the language server possibly with loaders from graphql utils not playing nicely with v17, but tricky for me to debug…

All end to end tests are passing however, so this is a start, but any help appreciated. :)

@yaacovCR yaacovCR force-pushed the new-incremental-format branch 4 times, most recently from c16072b to 2a8fb24 Compare August 13, 2024 06:08
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 10.63830% with 42 lines in your changes missing coverage. Please review.

Project coverage is 67.28%. Comparing base (1c1d2b5) to head (5d0470c).

Files Patch % Lines
packages/graphiql-react/src/execution.tsx 2.38% 41 Missing ⚠️
packages/graphiql/test/schema.js 80.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3682      +/-   ##
==========================================
- Coverage   67.60%   67.28%   -0.32%     
==========================================
  Files         121      121              
  Lines        6968     7007      +39     
  Branches     2251     2260       +9     
==========================================
+ Hits         4711     4715       +4     
- Misses       2240     2275      +35     
  Partials       17       17              
Files Coverage Δ
packages/graphiql/test/schema.js 38.57% <80.00%> (+2.20%) ⬆️
packages/graphiql-react/src/execution.tsx 11.11% <2.38%> (-2.28%) ⬇️

@yaacovCR
Copy link
Contributor Author

🦋 Changeset detected

Latest commit: 205980c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@graphiql/react Minor
graphiql Minor
@graphiql/plugin-code-exporter Major
@graphiql/plugin-explorer Major
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

not sure what these other packages are and why they are going to be released as a breaking change?

@yaacovCR yaacovCR changed the title support new incremental delivery format upgrade to v17-alpha Aug 13, 2024
@yaacovCR yaacovCR changed the title upgrade to v17-alpha upgrade to graphql v17 including support for new incremental delivery format Aug 13, 2024
@dimaMachina dimaMachina merged commit 6c9f0df into graphql:main Aug 13, 2024
14 checks passed
@acao acao mentioned this pull request Aug 13, 2024
@yaacovCR yaacovCR deleted the new-incremental-format branch August 13, 2024 22:41
dimaMachina added a commit that referenced this pull request Aug 13, 2024
* upgrade to graphql v17 including support for new incremental delivery format (#3682)

* add support for new incremental delivery format

* fix vitest

* add patch for graphql-js bug

submittes upstream with test at graphql/graphql-js#4160

* apply feedback

* move patches!

* some fixes

* fix netlify/cypress

* remove export

* update changeset

---------

Co-authored-by: Dimitri POSTOLOV <[email protected]>

* Version Packages (#3715)

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* prettier

* fix e2e

* update pr-graphql-compat-check.yml

* prettier

---------

Co-authored-by: Yaacov Rydzinski <[email protected]>
Co-authored-by: Rikki Schulte <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@acao
Copy link
Member

acao commented Sep 10, 2024

@dimaMachina next time, please do not merge a PR like this without asking. I need to manually revert many changes for us to return to spec compliance

@@ -0,0 +1,26 @@
diff --git a/node_modules/graphql/execution/execute.js b/node_modules/graphql/execution/execute.js
Copy link
Member

Choose a reason for hiding this comment

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

@dimaMachina again, a reminder that this patch is being published to users. we should not be patching graphql before publishing as a reference ecosystem

Copy link
Collaborator

Choose a reason for hiding this comment

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

This patch was not published to users

@acao acao mentioned this pull request Sep 27, 2024
5 tasks
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.

3 participants