Skip to content

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

Closed
IvanGoncharov wants to merge 8 commits intomainfrom
defer-stream
Closed

Add support for defer and stream directives (Feedback is welcome)#2839
IvanGoncharov wants to merge 8 commits intomainfrom
defer-stream

Conversation

@IvanGoncharov
Copy link
Copy Markdown
Member

@IvanGoncharov IvanGoncharov commented Nov 5, 2020 •
edited by robrichard
Loading

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 [email protected] 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
tgriesser
tgriesser reviewed Dec 5, 2020
View reviewed changes
@tgriesser tgriesser mentioned this pull request Dec 7, 2020
Closed
@edno edno mentioned this pull request Dec 7, 2020
Open
@yaacovCR
Copy link
Copy Markdown
Contributor

yaacovCR commented Dec 31, 2020

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

graphql-js/src/index.d.ts

Lines 54 to 58 in 163f62a

specifiedDirectives,
GraphQLIncludeDirective,
GraphQLSkipDirective,
GraphQLDeprecatedDirective,
GraphQLSpecifiedByDirective,

Maybe add:

  GraphQLDeferDirective,
  GraphQLSkipDirective,

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

graphql-js/src/type/index.d.ts

Lines 125 to 131 in 163f62a

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

@robrichard
Copy link
Copy Markdown
Contributor

robrichard commented Dec 31, 2020

@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
Copy Markdown

linux-foundation-easycla bot commented Feb 9, 2021 •
edited
Loading

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
Copy Markdown
Contributor

yaacovCR commented Mar 16, 2021 •
edited
Loading

cross-posting #2848 (comment)

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

@robrichard
Copy link
Copy Markdown
Contributor

robrichard commented Mar 16, 2021 •
edited
Loading

@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
Copy Markdown
Contributor

yaacovCR commented Mar 19, 2021

#2975

@KatFishSnake
Copy link
Copy Markdown

KatFishSnake commented May 14, 2021

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

@IvanGoncharov what is blocking you guys from merging it?

@KatFishSnake KatFishSnake mentioned this pull request May 14, 2021
Closed
4 tasks
@acao
Copy link
Copy Markdown
Member

acao commented Jun 1, 2021 •
edited
Loading

@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 acao mentioned this pull request Jun 1, 2021
Merged
5 tasks
@acao
Copy link
Copy Markdown
Member

acao commented Jun 1, 2021 •
edited
Loading

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.

Sign in to view
@robrichard

This comment has been minimized.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@IvanGoncharov

This comment has been minimized.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@IvanGoncharov

This comment has been minimized.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@IvanGoncharov

This comment has been minimized.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@IvanGoncharov

This comment has been minimized.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@IvanGoncharov

This comment has been minimized.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@robrichard

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 27, 2022

@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

@brainkim brainkim mentioned this pull request May 4, 2022
Closed
@ardatan ardatan mentioned this pull request May 17, 2022
Closed
@n1ru4l
Copy link
Copy Markdown
Contributor

n1ru4l commented May 23, 2022

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

@robrichard

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Copy Markdown

github-actions bot commented May 23, 2022

@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
@lilianammmatos @robrichard
Add @defer directive to schema
b371e53
@robrichard
Implement support for @defer directive
934cc82
@robrichard
Add @stream directive to schema
d212612
@robrichard
Implement support for @stream directive
2abfffe 
# Conflicts:
#	src/execution/execute.ts
#	src/validation/index.d.ts
#	src/validation/index.ts
@robrichard
add defer/stream support for subscriptions (#7)
07c5b61 
# Conflicts:
#	src/subscription/subscribe.ts
@robrichard
Return underlying AsyncIterators when execute result is returned (#2843)
05f1903 
# Conflicts:
#	src/execution/execute.ts
@robrichard
fix(race): concurrent next calls with defer/stream (#2975)
19be33e 
* fix(race): concurrent next calls

* refactor test

* use invariant

* disable eslint error

* fix
@robrichard
Add defer/stream tutorial
35a28cd
@robrichard robrichard mentioned this pull request Jun 23, 2022
Merged
@robrichard
Copy link
Copy Markdown
Contributor

robrichard commented Jun 23, 2022

Continuing in new PR #3659

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robrichard robrichard robrichard left review comments

@yaacovCR yaacovCR yaacovCR left review comments

+3 more reviewers

@tgriesser tgriesser tgriesser left review comments

@n1ru4l n1ru4l n1ru4l left review comments

@danielrearden danielrearden danielrearden left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@IvanGoncharov @yaacovCR @robrichard @KatFishSnake @acao @glasser @n1ru4l @tgriesser @danielrearden @lilianammmatos