align return types from execution and subscription by yaacovCR · Pull Request #3620 · graphql/graphql-js ·

with respect to possible promises

motivations:

no need to enter event loop unnecessarily
further step toward integration of subscribe workflow into execute

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	`6714bbe`
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62a24882d5e85b000814bddd
😎 Deploy Preview	https://deploy-preview-3620--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Two test groups use essentially the same logic to set up a subscription with an improper subscribeFn, testing both `subscribe` and `createSourceEventStream`. This PR extracts the duplicated logic into a single common `subscribeWithBadFn` function. For convenience, the common function is typed to appropriately return a `Promise<ExecutionResult>` rather than a `Promise<ExecutionResult | AsyncGenerator<...>>`). Because the `subscribeFn` is expected to be "bad," an `AsyncGenerator` should never be returned. If a valid `subscribeFn` is mistakenly passed, an assertion failure is triggered. extracted from graphql#3620

because this depends on #3630 the diff to review (until #3630 is merged) would be here:

yaacovCR/graphql-js@extract...align

Two test groups use essentially the same logic to set up a subscription with an improper subscribeFn, testing both `subscribe` and `createSourceEventStream`. This PR extracts the duplicated logic into a single common `subscribeWithBadFn` function. For convenience, the common function is typed to appropriately return a `Promise<ExecutionResult>` rather than a `Promise<ExecutionResult | AsyncGenerator<...>>`). Because the `subscribeFn` is expected to be "bad," an `AsyncGenerator` should never be returned. If a valid `subscribeFn` is mistakenly passed, an assertion failure is triggered. extracted from #3620

with respect to possible promises

@IvanGoncharov

@IvanGoncharov @graphql/graphql-js-reviewers

This has been rebased and is re-ready for review.

src/execution/subscribe.ts

src/execution/__tests__/subscribe-test.ts

= remove unnecessary waits = simply subscribeWithBadFn

until it is asserted as eventStream

See further discussion with respect to advantages of using repeaters about general anti pattern of Promise<AsyncIterator>

@brainkim

@brainkim

yaacovCR requested a review from a team May 31, 2022 12:22

yaacovCR added the PR: breaking change 💥implementation requires increase of "major" version numberlabel May 31, 2022

yaacovCR force-pushed the align branch 2 times, most recently from a735422 to b0e56f2 Compare June 7, 2022 07:45

yaacovCR mentioned this pull request Jun 7, 2022
subscribe-test: extract subscribeWithBadFn function #3630
Merged

yaacovCR force-pushed the align branch 2 times, most recently from d89e2de to 7f214ea Compare June 7, 2022 10:17

yaacovCR force-pushed the align branch 2 times, most recently from 2321ee8 to 3b27fb6 Compare June 7, 2022 11:40

align return types from execution and subscription

713480f
with respect to possible promises

yaacovCR force-pushed the align branch from 3b27fb6 to 713480f Compare June 8, 2022 21:22

fix typo

e9700ca