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

fix error display for websocket fetchers #2535

Merged
merged 6 commits into from
Aug 5, 2022
Merged

Conversation

thomasheyenbrock
Copy link
Collaborator

@thomasheyenbrock thomasheyenbrock commented Jul 4, 2022

Fixes #2534

The issue is caused by the default ws-fetcher which wraps joins multiple error messages and wraps them in a new error.

Open TODOs:

  • add changeset
  • write some tests for displaying correctly formatted errors in general

@thomasheyenbrock thomasheyenbrock requested a review from acao July 4, 2022 07:57
@changeset-bot
Copy link

changeset-bot bot commented Jul 4, 2022

🦋 Changeset detected

Latest commit: 37fa61e

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

This PR includes changesets to release 3 packages
Name Type
@graphiql/toolkit Patch
@graphiql/react Patch
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

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2022

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

@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #2535 (37fa61e) into main (2d91916) will increase coverage by 3.82%.
The diff coverage is 23.84%.

@@            Coverage Diff             @@
##             main    #2535      +/-   ##
==========================================
+ Coverage   65.70%   69.52%   +3.82%     
==========================================
  Files          85       71      -14     
  Lines        5106     4204     -902     
  Branches     1631     1410     -221     
==========================================
- Hits         3355     2923     -432     
+ Misses       1747     1276     -471     
- Partials        4        5       +1     
Impacted Files Coverage Δ
packages/codemirror-graphql/src/lint.ts 100.00% <ø> (ø)
packages/codemirror-graphql/src/results/mode.ts 47.05% <ø> (ø)
packages/codemirror-graphql/src/utils/hintList.ts 95.65% <ø> (ø)
...ckages/codemirror-graphql/src/utils/mode-indent.ts 0.00% <0.00%> (ø)
packages/codemirror-graphql/src/variables/mode.ts 79.48% <ø> (ø)
packages/graphiql-react/src/editor/whitespace.ts 100.00% <ø> (ø)
packages/graphiql-react/src/utility/debounce.ts 0.00% <0.00%> (ø)
packages/graphiql-react/src/editor/tabs.ts 5.66% <5.66%> (ø)
packages/codemirror-graphql/src/variables/lint.ts 47.61% <66.66%> (+0.63%) ⬆️
packages/codemirror-graphql/src/hint.ts 94.73% <100.00%> (ø)
... and 99 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@acao
Copy link
Member

acao commented Jul 4, 2022

yeah i think this is a longtime issue, maybe not a regression, so feel free to fix it in 2.0.0. will take a look at this later today!

@thomasheyenbrock
Copy link
Collaborator Author

We could also fix that with a new patch in v1 as we decided not to do the prereleases and this does not contain breaking changes 😉

@acao
Copy link
Member

acao commented Jul 8, 2022

ah yes of course! i was hoping to make it easier on you but that's exactly what we agreed to isn't it, i forgot we have a path for this now! so nice to have, looking forward to using pre-release more often

it looks like there is a new cypress failure here, are you familar with this one?

https://github.com/graphql/graphiql/runs/7176376103?check_suite_focus=true#step:8:412

@acao
Copy link
Member

acao commented Jul 13, 2022

Looks good! I approved, just need to fix the e2e suite

Screenshot 2022-07-13 at 16 01 47

@acao
Copy link
Member

acao commented Jul 14, 2022

oh my bad i see the write tests task

@thomasheyenbrock thomasheyenbrock merged commit ea732ea into main Aug 5, 2022
@thomasheyenbrock thomasheyenbrock deleted the issues/2534 branch August 5, 2022 11:24
@acao acao mentioned this pull request Aug 5, 2022
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.

Getting an Error for Invalid Subscription Requests
2 participants