fix(ws): Subscriptions async iterator completes and better error handling #1841
Conversation
🦋 Changeset detectedLatest commit: 933444a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
| }, | ||
| }); | ||
| return { | ||
| [Symbol.asyncIterator]() { |
There was a problem hiding this comment.
Note that Symbol.asyncIterator is not available in Safari 14 (iOS), that is why I create push-pull-async-iterable-iterator in the first place.
There was a problem hiding this comment.
Only reason why I replaced it is #1827 (comment).
There was a problem hiding this comment.
Also, shouldn't the transpilation be handled by the user? If you'd like to support Safari 14 on iOS, simply polyfill the async iterator with Babel on build step. @acao what is your take on this?
There was a problem hiding this comment.
I have no strong feelings about this. For myself, I can say that I am shipping an app with Safari 14 support and try to avoid babel transforms where possible, that is why I created the lib, later I had the need to transform sinks into AsyncIterables as they are easier to map/filter etc so I added it to the library. If GraphiQL does not need to rely on push-pull-async-iterable-iterator I am totally fine with it.
There was a problem hiding this comment.
Reverted back to using the push-pull-async-iterable-iterator following v2.1.3.
Codecov Report
@@ Coverage Diff @@
## main #1841 +/- ##
==========================================
+ Coverage 65.70% 65.81% +0.10%
==========================================
Files 85 85
Lines 5106 5116 +10
Branches 1631 1625 -6
==========================================
+ Hits 3355 3367 +12
+ Misses 1747 1745 -2
Partials 4 4
Continue to review full report at Codecov.
|
|
Fixed with |
|
Hmm, however, there is now another issue at hand. The error handling here: https://github.com/enisdenjo/graphiql/blob/4ed8102a0d8157ba1e89dc438e9236450cdd64b6/packages/graphiql-toolkit/src/create-fetcher/lib.ts#L99-L115 It worked following the recipe, but reverting back to using I have no stack trace sadly, could there be an uncaught promise somewhere in |
|
Note @n1ru4l, the iterator from |
enisdenjo
changed the title
graphql-ws subscriptions and better error handling
|
Before merging please also reflect whether push-pull-async-iterable-iterator is a necessary dependency @acao 😇 |
|
Appreciate y’all putting so much into this already! Please ping me directly in the future if you have a PR ready for me as I have notifications disabled, and only tend to check on the weekends now that I’m still trying to take a break from maintaining I don’t have time to look into whether or not that is a necessary dependency @n1ru4l , is it? Should we just merge and release this patch bugfix, and then drop it in a minor release as a follow up at least? lastly can you... you actually created the changeset... 🥺😍 omg you guys are the best! |
Closes #1827