fix: onSubscribeError should be invoked #3196

n1ru4l · 2024-03-08T09:00:42Z

I noticed that the onSubscribeError error hook is not invoked due to how graphql-tools/executor does execution.

See ardatan/graphql-tools#5965, for the prerequisite changes with in tools/executor

changeset-bot · 2024-03-08T09:00:45Z

🦋 Changeset detected

Latest commit: b16074b

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

This PR includes changesets to release 24 packages

Name	Type
graphql-yoga	Minor
@graphql-yoga/nestjs	Major
@graphql-yoga/render-graphiql	Major
@graphql-yoga/plugin-apollo-inline-trace	Major
@graphql-yoga/plugin-apq	Major
@graphql-yoga/plugin-csrf-prevention	Major
@graphql-yoga/plugin-defer-stream	Major
@graphql-yoga/plugin-disable-introspection	Major
@graphql-yoga/plugin-graphql-sse	Major
@graphql-yoga/plugin-jwt	Major
@graphql-yoga/plugin-persisted-operations	Major
@graphql-yoga/plugin-prometheus	Major
@graphql-yoga/plugin-response-cache	Major
@graphql-yoga/plugin-sofa	Major
apollo-federation-gateway-with-yoga	Patch
apollo-subgraph-with-yoga	Patch
graphql-lambda	Patch
cloudflare-advanced	Patch
cloudflare	Patch
functions	Patch
hackernews	Patch
nextjs-app	Patch
hello-world-benchmark	Patch
@graphql-yoga/nestjs-federation	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 · 2024-03-08T09:03:31Z

Apollo Federation Subgraph Compatibility Results

Federation 1 Support Federation 2 Support

`_service`	🟢
`@key (single)`	🟢
`@key (multi)`	🟢
`@key (composite)`	🟢
`repeatable @key`	🟢
`@requires`	🟢
`@provides`	🟢
`federated tracing`	🟢

`@link`	🟢
`@shareable`	🟢
`@tag`	🟢
`@override`	🟢
`@inaccessible`	🟢
`@composeDirective`	🟢
`@interfaceObject`	🟢

Learn more:

github-actions · 2024-03-08T09:05:53Z

💻 Website Preview

The latest changes are available as preview in: https://f7386c56.graphql-yoga.pages.dev

github-actions · 2024-03-08T09:06:00Z

✅ Benchmark Results

     ✓ no_errors{mode:graphql}
     ✓ expected_result{mode:graphql}
     ✓ no_errors{mode:graphql-jit}
     ✓ expected_result{mode:graphql-jit}
     ✓ no_errors{mode:graphql-response-cache}
     ✓ expected_result{mode:graphql-response-cache}
     ✓ no_errors{mode:graphql-no-parse-validate-cache}
     ✓ expected_result{mode:graphql-no-parse-validate-cache}

     checks.......................................: 100.00% ✓ 406678      ✗ 0     
     data_received................................: 1.6 GB  14 MB/s
     data_sent....................................: 82 MB   684 kB/s
     http_req_blocked.............................: avg=1.45µs   min=952ns    med=1.3µs    max=4.58ms   p(90)=1.89µs   p(95)=2.09µs  
     http_req_connecting..........................: avg=2ns      min=0s       med=0s       max=130.88µs p(90)=0s       p(95)=0s      
     http_req_duration............................: avg=379.33µs min=205.42µs med=337.12µs max=16.54ms  p(90)=541.41µs p(95)=560.49µs
       { expected_response:true }.................: avg=379.33µs min=205.42µs med=337.12µs max=16.54ms  p(90)=541.41µs p(95)=560.49µs
     ✓ { mode:graphql-jit }.......................: avg=281.45µs min=205.42µs med=261.45µs max=14.51ms  p(90)=289.77µs p(95)=299.92µs
     ✓ { mode:graphql-no-parse-validate-cache }...: avg=566.82µs min=448.5µs  med=541.21µs max=11.68ms  p(90)=580.57µs p(95)=610.9µs 
     ✓ { mode:graphql-response-cache }............: avg=360.76µs min=287.59µs med=341.6µs  max=16.54ms  p(90)=370.88µs p(95)=380.58µs
     ✓ { mode:graphql }...........................: avg=375.27µs min=280.56µs med=343.99µs max=13.18ms  p(90)=383.47µs p(95)=424.73µs
     http_req_failed..............................: 0.00%   ✓ 0           ✗ 203339
     http_req_receiving...........................: avg=32.48µs  min=16.04µs  med=32.47µs  max=6.04ms   p(90)=38.23µs  p(95)=40.31µs 
     http_req_sending.............................: avg=8.17µs   min=5.6µs    med=7.32µs   max=3.99ms   p(90)=10.72µs  p(95)=11.36µs 
     http_req_tls_handshaking.....................: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting.............................: avg=338.68µs min=175.04µs med=297.21µs max=16.47ms  p(90)=500.74µs p(95)=517.57µs
     http_reqs....................................: 203339  1694.468931/s
     iteration_duration...........................: avg=585.49µs min=369.93µs med=540.04µs max=16.79ms  p(90)=750.97µs p(95)=773.13µs
     iterations...................................: 203339  1694.468931/s
     vus..........................................: 1       min=1         max=1   
     vus_max......................................: 2       min=2         max=2

github-actions · 2024-03-08T09:06:27Z

🚀 Snapshot Release (`alpha`)

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

Package	Version	Info
`graphql-yoga-cloud-run-guide`	`3.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/apollo-link`	`3.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/urql-exchange`	`3.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`graphql-yoga`	`5.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/nestjs`	`3.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/nestjs-federation`	`3.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/plugin-apollo-inline-trace`	`3.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/plugin-apq`	`3.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/plugin-csrf-prevention`	`3.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/plugin-defer-stream`	`3.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/plugin-disable-introspection`	`2.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/plugin-graphql-sse`	`3.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/plugin-jwt`	`2.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/plugin-persisted-operations`	`3.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/plugin-prometheus`	`4.1.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/plugin-response-cache`	`3.4.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/plugin-sofa`	`3.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎
`@graphql-yoga/render-graphiql`	`5.2.0-alpha-20240312090652-b16074bb`	npm ↗︎ unpkg ↗︎

EmrysMyrddin · 2024-03-08T15:17:59Z

packages/graphql-yoga/src/server.ts

@@ -475,6 +476,32 @@ export class YogaServer<
        this.logger.debug(`Processing GraphQL Parameters done.`);
      }

+      /** Ensure that error thrown from subscribe is sent to client */
+      if (result && Symbol.asyncIterator in result) {


You don't use isAsyncResult util we have ?

The typing of this function is incorrect result can never be async iterable, if we want to do this change we need to refactor and fix the typings (which I will do ofc).

@EmrysMyrddin see 656ea8f (#3196)

EmrysMyrddin · 2024-03-08T15:21:18Z

Why do we want to send internal event source errors to the client ? Shouldn't we just abort the connection at this point ?
Perhaps allowing cutom close code but I'm not sure if we should send the error ?

Is there and RFC for this ?

n1ru4l · 2024-03-09T10:26:57Z

Why do we want to send internal event source errors to the client ?

You could do an auth/access check that should be sent to the client for example.

There is unfortunately no RFC or clarity...

make that test fail

4be053a

this fixes it

59d897e

n1ru4l added 2 commits March 8, 2024 10:19

test: subscription plugin hooks are invoked

93e0d22

test: error masking for subscriptions

5edf58c

n1ru4l force-pushed the failing-test branch from d30ba99 to 8771855 Compare March 8, 2024 14:05

fix: ensure onSubscribeError is invoked

983686d

n1ru4l force-pushed the failing-test branch from 8771855 to 983686d Compare March 8, 2024 14:07

this version works

0158a1c

n1ru4l force-pushed the failing-test branch from 55bef8d to 0158a1c Compare March 8, 2024 14:30

n1ru4l changed the title ~~test: failing subscription event stream~~ fix: onSubscribeError should be invoked Mar 8, 2024

EmrysMyrddin reviewed Mar 8, 2024

View reviewed changes

feat: result handling

656ea8f

n1ru4l force-pushed the failing-test branch from 52f6f4b to 656ea8f Compare March 11, 2024 09:17

n1ru4l added 2 commits March 11, 2024 10:21

no need to wrap; this is responsibility of executor

32810eb

ok

458a854

n1ru4l force-pushed the failing-test branch from 4c2678d to 458a854 Compare March 11, 2024 11:05

github-actions bot and others added 4 commits March 11, 2024 11:10

chore(dependencies): updated changesets for modified dependencies

10a142e

Use mapAsyncIterator instead

ff22df0

Use error handler

9b8af74

Update executor version

fb914e2

ardatan approved these changes Mar 11, 2024

View reviewed changes

github-actions bot and others added 2 commits March 11, 2024 12:50

chore(dependencies): updated changesets for modified dependencies

8dfdf08

Test if error masking fn is called once

1a07d35

github-actions bot and others added 2 commits March 11, 2024 13:00

chore(dependencies): updated changesets for modified dependencies

4e5a8d8

changeset: onParams setResult

b16074b

n1ru4l merged commit 71db754 into main Mar 12, 2024
27 of 29 checks passed

n1ru4l deleted the failing-test branch March 12, 2024 11:02

theguild-bot mentioned this pull request Mar 12, 2024

Upcoming Release Changes #3184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: onSubscribeError should be invoked #3196

fix: onSubscribeError should be invoked #3196

n1ru4l commented Mar 8, 2024 •

edited

Loading

changeset-bot bot commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

EmrysMyrddin Mar 8, 2024

n1ru4l Mar 9, 2024

n1ru4l Mar 11, 2024

EmrysMyrddin commented Mar 8, 2024

n1ru4l commented Mar 9, 2024 •

edited

Loading

fix: onSubscribeError should be invoked #3196

fix: onSubscribeError should be invoked #3196

Conversation

n1ru4l commented Mar 8, 2024 • edited Loading

changeset-bot bot commented Mar 8, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Mar 8, 2024 • edited Loading

Apollo Federation Subgraph Compatibility Results

github-actions bot commented Mar 8, 2024 • edited Loading

💻 Website Preview

github-actions bot commented Mar 8, 2024 • edited Loading

✅ Benchmark Results

github-actions bot commented Mar 8, 2024 • edited Loading

🚀 Snapshot Release (alpha)

EmrysMyrddin Mar 8, 2024

Choose a reason for hiding this comment

n1ru4l Mar 9, 2024

Choose a reason for hiding this comment

n1ru4l Mar 11, 2024

Choose a reason for hiding this comment

EmrysMyrddin commented Mar 8, 2024

n1ru4l commented Mar 9, 2024 • edited Loading

n1ru4l commented Mar 8, 2024 •

edited

Loading

changeset-bot bot commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

🚀 Snapshot Release (`alpha`)

n1ru4l commented Mar 9, 2024 •

edited

Loading