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

Add support for defer and stream directives (Feedback is welcome) #2839

Closed
wants to merge 8 commits into from

Conversation

IvanGoncharov
Copy link
Member

@IvanGoncharov IvanGoncharov commented Nov 5, 2020

The implementation in this pull request is a reference implementation of the specification proposal outlined in graphql/graphql-spec#742. It is no longer compatible with with the @defer and @stream implementation in Relay.

This branch is published to npm as version graphql@15.4.0-experimental-stream-defer.1 under the tag graphql@experimental-stream-defer. We encourage anyone interested in this feature to try out this version and report any feedback, issues, or potential improvements to the API. This feedback will help the working group gain confidence to move forward with standardization.

Developed and maintained by @lilianammmatos @robrichard

Depends on #2757

Continued in new pr: #3659

@IvanGoncharov IvanGoncharov added spec RFC Implementation of a proposed change to the GraphQL specification PR: feature 🚀 requires increase of "minor" version number labels Nov 5, 2020
@IvanGoncharov IvanGoncharov changed the title Add support for defer and stream directives (Feedback is welcomed) Add support for defer and stream directives (Feedback is welcome) Nov 5, 2020
src/graphql.d.ts Outdated Show resolved Hide resolved
@yaacovCR
Copy link
Contributor

Hi team! Thanks again for all the hard work, a quick, tiny suggestion:

specifiedDirectives,
GraphQLIncludeDirective,
GraphQLSkipDirective,
GraphQLDeprecatedDirective,
GraphQLSpecifiedByDirective,

Maybe add:

  GraphQLDeferDirective,
  GraphQLSkipDirective,

Currently my editor automatically has me importing from graphql/type, which has the change:

specifiedDirectives,
GraphQLIncludeDirective,
GraphQLSkipDirective,
GraphQLDeferDirective,
GraphQLStreamDirective,
GraphQLDeprecatedDirective,
GraphQLSpecifiedByDirective,

@robrichard
Copy link
Contributor

@yaacovCR thanks, updated now.

@robrichard robrichard force-pushed the defer-stream branch 3 times, most recently from cd9c8c6 to 1c4940a Compare January 22, 2021 21:02
Base automatically changed from master to main January 27, 2021 11:10
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 9, 2021

CLA Signed

The committers are authorized under a signed CLA.

@robrichard robrichard force-pushed the defer-stream branch 6 times, most recently from 4be6bf2 to 24330c0 Compare February 16, 2021 14:33
@yaacovCR
Copy link
Contributor

yaacovCR commented Mar 16, 2021

cross-posting #2848 (comment)

I believe this is a bug in the Set<Promise<result>> => AsyncIterableIterator<result> functionality

@robrichard
Copy link
Contributor

robrichard commented Mar 16, 2021

@yaacovCR I think this is a bug. The dispatcher is not expecting next() to be called before the previous next() is resolved. I'll take a look when I have some time.

@yaacovCR
Copy link
Contributor

#2975

@KatFishSnake
Copy link

Any updates here?, anyways we can track the progress?

@IvanGoncharov what is blocking you guys from merging it?

@acao
Copy link
Member

acao commented Jun 1, 2021

@KatFishSnake they were waiting for the typescript migration, and as I understand, will be releasing with graphql@16. So once all the conflicts are reconciled here it should be close! I can't decide whether this leaves RFC myself but I can imagine it will.

@acao
Copy link
Member

acao commented Jun 1, 2021

So, I began the process of converting this PR to typescript and got most of the way through the third commit!!

I'm going to go to sleep and enjoy more of my vacation, but I just wanted to confirm that this will be a somewhat involved & worthwhile effort just for the language conversion work. If someone can start on this effort now and see it through that would be rad! Converting flow to typescript isn't as hard as it sounds I promise!

then, as I understand, there are incoming spec changes? or was that to just the transport spec?

CC @robrichard @freiksenet @IvanGoncharov @leebyron @saihaj

@github-actions

This comment was marked as outdated.

@robrichard

This comment has been minimized.

@github-actions
Copy link

@github-actions publish-pr-on-npm

@robrichard The latest changes of this PR are available on NPM as
graphql@16.4.0-canary.pr.2839.e3a8069cfaa6406186314b62aced6487f417a2e6
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-2839

@n1ru4l
Copy link
Contributor

n1ru4l commented May 23, 2022

@robrichard Can you please rebase upon main and release a new canary?

@robrichard

This comment has been minimized.

@github-actions
Copy link

@github-actions publish-pr-on-npm

@robrichard The latest changes of this PR are available on NPM as
graphql@17.0.0-alpha.1.canary.pr.2839.db4d0cdea30214fb7bb00724b7827708ca5de8a5
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-2839

lilianammmatos and others added 8 commits June 23, 2022 14:21
# Conflicts:
#	src/execution/execute.ts
#	src/validation/index.d.ts
#	src/validation/index.ts
# Conflicts:
#	src/subscription/subscribe.ts
* fix(race): concurrent next calls

* refactor test

* use invariant

* disable eslint error

* fix
@robrichard
Copy link
Contributor

Continuing in new PR #3659

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number spec RFC Implementation of a proposed change to the GraphQL specification stream/defer Issues/PRs related to experimental steam/defer support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants