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

implement correct merging of incremental respones (@defer/@stream) #3580

Merged
merged 3 commits into from
May 7, 2024

Conversation

thomasheyenbrock
Copy link
Collaborator

Hello fellow GraphiQL friends 👋

We recently stumbled over the fact that GraphiQL does not yet implement the latest incremental response format for defer/stream, concretely the incremental payload property is not handled.

This PR cleans up the merge logic and makes it work for any incremental response payload. (Big inspiration taken from graphql-tools/utils, but if felt overkill to add the whole package just for this one utility, and since this is the official reference implementation for a GraphQL IDE it felt appropriate to have that utility as first-class citizen in our code.)

Some before-after illustrations:

Feature Before After
Defer CleanShot 2024-04-10 at 11 16 14 CleanShot 2024-04-10 at 11 16 55
Stream CleanShot 2024-04-10 at 11 16 30 CleanShot 2024-04-10 at 11 17 13

Copy link

changeset-bot bot commented Apr 10, 2024

🦋 Changeset detected

Latest commit: 8acbecf

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/plugin-code-exporter Major
@graphiql/plugin-explorer Major
graphiql Patch

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
Copy link
Contributor

Neat!

Something I've been wondering...

The new response format has each payload containing supplemental data/items, but also metadata about the incremental stream in the form of pending/completed notifications.

With the UI strategy of merging incremental data into the initial response, that information is not currently shown. It also makes it impossible to tell from the final result what actually happened; the user has to watch carefully as the process unfolds.

Might we want to model display of incremental results similar to subscriptions in which we just display the different payloads separately.

Maybe as a toggle-able separate useful option?

@thomasheyenbrock
Copy link
Collaborator Author

I've been pondering about that as well. GraphiQL currently has no good interface for long-running requests that return more than one payload.

  • For subscriptions you only ever see the latest payload (GraphiQL does replace the result contents each time a payload is received)
  • For defer/stream you only see the latest combined result, but not the increments (like you mentioned @yaacovCR)

This would need some bigger re-architecting for how the internal state is managed as well as the UI. I see that as outside of the scope of this PR which is just intending to fix the most pressing issues with defer/stream.

@yaacovCR
Copy link
Contributor

Oh => I suppose I am simply mistaken about subscriptions. Sorry about that!

@acao
Copy link
Member

acao commented Apr 12, 2024

yes this UI change to results for subscriptions has been something we've been hoping to execute for a while. And incremental delivery with stream creates another UI case, where we want to see the final result but also a log of the requests that formed that result. We hope to expose plugins for custom tabs in results, and to allow overriding the results pane, and the other idea we had years ago was for these results plugins could register and display for specific response types automatically.

@acao
Copy link
Member

acao commented May 1, 2024

@thomasheyenbrock looks awesome! just need to fix this ts bug:

src/execution.tsx(364,7): error TS2357: The operand of an increment or decrement operator must be a variable or a property access.

and get the unit and related cypress tests passing. if you like, i can show you how to enable to e2e tests for defer and stream, if this has been merged into graphql by now? previously, the e2e tests for defer and stream were designed to be run against the special release tag version for years, so it's just a small tweak to cypress to get them to run for the stable graphql release version

Copy link

codecov bot commented May 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 55.31%. Comparing base (29e99a8) to head (8acbecf).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3580      +/-   ##
==========================================
- Coverage   55.33%   55.31%   -0.03%     
==========================================
  Files         115      115              
  Lines        5349     5351       +2     
  Branches     1450     1451       +1     
==========================================
  Hits         2960     2960              
- Misses       1963     1965       +2     
  Partials      426      426              
Files Coverage Δ
packages/graphiql-react/src/execution.tsx 13.15% <0.00%> (-0.24%) ⬇️

Copy link
Contributor

github-actions bot commented May 6, 2024

The latest changes of this PR are available as canary in npm (based on the declared changesets):

@thomasheyenbrock
Copy link
Collaborator Author

Fixed the TS and linter issues 👍 (sorry for taking so long to come back to this). Will have another look at e2e tests tomorrow. The current ones are passing but I also remember that we had some for defer/stream, so worth a look.

@acao
Copy link
Member

acao commented May 7, 2024

@thomasheyenbrock awesome! yes the e2e tags are designed to reference the graphql tag in package.json, and were previously run in the graphql version test matrix which i think we disabled. if defer and stream are in the stable graphql release we can get rid of that logic

@thomasheyenbrock
Copy link
Collaborator Author

Yes, just found the same thing 👍 will merge this for now and look into testing defer/stream reliably a bit more in detail separately

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.

4 participants