Support defer/stream spec proposal #3121

robrichard · 2020-07-07T17:30:21Z

This change will allow Relay to support @defer/@stream in the form that is currently being proposed to the GraphQL WG:

Draft Spec: graphql/graphql-spec#742
GraphQL-js: graphql/graphql-js#2319

This works by allowing either initial_count or initialCount as an argument for @stream. Whichever one is passed will be forwarded to the GraphQL server.

Additionally, either extensions.is_final: true or hasNext: false will be used to track final payloads.

josephsavona · 2020-07-07T19:33:13Z

Awesome, thanks for sending this! Any interest in adding equivalent support to the rust version of the compiler? :-)

robrichard · 2020-07-07T21:25:36Z

@josephsavona I added support to the rust compiler. I did not run it against any actual code though, just worked through the tests.

maraisr · 2020-07-07T21:26:40Z

compiler/crates/graphql-transforms/src/transform_connections.rs

            for arg in &connection_directive.arguments {
                if arg.name.item == DEFER_STREAM_CONSTANTS.if_arg
                    || arg.name.item == DEFER_STREAM_CONSTANTS.initial_count_arg
+                    || arg.name.item == DEFER_STREAM_CONSTANTS.initial_count_arg_oss


I don't think this is the right approach. It should be either, not both. Have a look at how i'm currently supporting fb/oss for connection things.

https://github.com/facebook/relay/pull/3112/files#diff-86ef1b8ad53dcd24d2a88fabf01a1fc7R40

There is a few issues with it though, so still ironing out the kinks

My assumption is the schema you are working with is not likely to support both. In this case if you did pass both initialCount and initial_count to a @stream_connection directive, you will get a schema validation error that one of those args is invalid.

No sorry. Its more that both shouldn't be supported at the same time. As the spec will dictate the oss variant should be used. All I was saying, is to instead of or the 2, put it behind a compiler flag, such that OSS people only get the spec version.

Agree here: rather than have the code allow either argument name, this would ideally be a compiler config (similar to what we do for connections, which also have different naming internally and in OSS). Then the code would just check for that single name.

@josephsavona is the compiler config for the js compiler in the Relay repo? I only see the OSS names https://github.com/facebook/relay/blob/master/packages/relay-runtime/handlers/connection/ConnectionInterface.js#L55

Yeah we have an internal version of that file too, which uses FB's naming (basically the same but snake case). I think we can do the same thing, just put all the constants in a file similar to ConnectionInterface, and then we can maintain an internal version.

@josephsavona I updated the PR to add a config to the JS compiler similar to the ConnectionInterface. '

The hasNext => extensions.is_final is still a runtime check, since it is not a just a property name change (and the check is in relay-runtime not the compiler).

I also removed the rust change. @maraisr maybe you can help out and add it to your pr #3112?

See 1386365 for how we can add this to the config in the Relay Rust compiler (we could rename connectionConfig to schemaConfig or similar if we want to avoid bloat?)

josephsavona

Awesome, the changes here look good. We can make any final tweaks when we import. However, I do think we should wait until the final version of the spec is approved (just to avoid extra work on our side). Has that already happened?

robrichard · 2020-07-14T17:44:27Z

@josephsavona the spec is not approved. The consensus from the last GraphQL WG meeting was that the next step is to get @defer and @stream merged to graphql-js behind an experimental flag. The intention is to make it easier for people to try it out and start getting real word feedback from outside of Facebook and 1stdibs before finalizing the spec. Maybe it makes sense to get this merged once the related code is in GraphQL-js (graphql/graphql-js#2319)?

robrichard · 2020-11-12T19:32:30Z

@josephsavona @kassens official experimental branches of graphql-js with support for @defer and @stream are now being published. The working group is aligned on this approach but is waiting on community feedback before finalizing.

Do you think this can be merged so Relay users can easily start testing out @defer and @stream?

Still working on/trying to get help on the rust compiler, I can open a separate PR when that is ready.

sibelius · 2020-11-12T19:41:46Z

it would be great for the next Relay release

robrichard · 2020-12-21T15:14:43Z

@josephsavona @kassens I updated this PR to make the same change to the rust compiler. ~~The check failures are also failing on master~~, locally cargo build and cargo test are working as expected.

It was a little more involved than the ConnectionInterface since the codegen also needs to be passed the configurable DeferStreamInterface in a bunch of places. I'm still a Rust beginner so let me know if anything could be done in a better way.

chriserickson · 2021-06-17T21:27:13Z

Curious what the status of this PR is - it looks like it was being held until the spec was approved (which it has been now), but is now a bit stale and has conflicts. Definitely interested in using this and could help contribute.

chriserickson · 2021-06-17T21:34:21Z

Curious what the status of this PR is - it looks like it was being held until the spec was approved (which it has been now), but is now a bit stale and has conflicts. Definitely interested in using this and could help contribute.

Oh, I see, they are still experimental.

sorenhoyer · 2021-09-02T08:45:35Z

Would be great to have this merged soon, so we can start experimenting with defer and stream in Relay. Seems like Apollo Client has planned support for this in September. :-)

robrichard · 2021-12-17T00:07:57Z

@alunyov @rbalicki2 This branch up to date and working again. It's set up the same was as ConnectionInterface, a new DeferStreamInterface can be passed to the config, replacing the static DEFER_STREAM_CONSTANTS. The changes are somewhat large, but very repetitive, as the new interface needed to be passed to several transforms. I've separated the changes to relay-runtime, compiler code, test code, and test fixtures into separate commits which should make it easier to review.

alunyov · 2021-12-17T15:20:48Z

compiler/crates/relay-transforms/src/apply_transforms.rs

-        transform_connections(&program, &project_config.schema_config.connection_interface)
+        transform_connections(
+            &program,
+            &project_config.schema_config.connection_interface,


I think we should consider adding schema_config directly to the program.

We already have Arc<Schema> there. And I think it would be easier if we can just always have them together: schema and schema_config.

Let me try this internally.

Update: I could not come up with a better version of the Schema with Config. We have some some non-relay users of Programs/Schema, so adding a dependency on relay-config doesn't make sense for them. And adding something like a trait for Program is a bigger change.

Let's keep it for now, but maybe in this specific line: let's pass a &project_config.schema_config to the transform_connections instead of individual config options.

@alunyov I've updated transform_connections to accept schema_config

facebook-github-bot · 2021-12-20T14:51:40Z

@alunyov has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

alunyov · 2021-12-20T20:31:29Z

@robrichard, I have imported this PR internally. It still needs some additional handling to land it internally. Also, @rbalicki2 was working on this integration recently. I think, once Robert is back from the holidays, he may have some feedback.

But overall, this looks great, and I'm pretty optimistic that this will land soon. Thank you for your work!

# Conflicts: # packages/relay-runtime/store/RelayModernQueryExecutor.js

# Conflicts: # compiler/crates/relay-compiler/tests/compile_relay_artifacts/mod.rs # compiler/crates/relay-lsp/src/graphql_tools/mod.rs # compiler/crates/relay-typegen/tests/generate_flow/mod.rs # compiler/crates/relay-typegen/tests/generate_typescript/mod.rs

Tiedye · 2022-05-24T00:03:15Z

Any updates on this?

robrichard · 2023-10-06T17:13:09Z

Continued in #4467

robrichard force-pushed the spec-defer branch from 0c5c2fb to 9e9c8ab Compare July 7, 2020 17:41

facebook-github-bot added the CLA Signed label Jul 7, 2020

maraisr reviewed Jul 7, 2020

View reviewed changes

robrichard force-pushed the spec-defer branch from a5a48a6 to cd4a139 Compare July 14, 2020 15:09

josephsavona approved these changes Jul 14, 2020

View reviewed changes

robrichard force-pushed the spec-defer branch 3 times, most recently from 56df551 to 3d745e1 Compare November 6, 2020 22:18

robrichard force-pushed the spec-defer branch 2 times, most recently from a5aecaf to 76375df Compare December 21, 2020 14:57

robrichard force-pushed the spec-defer branch from 76375df to 03c75a3 Compare December 22, 2020 00:57

AlicanC mentioned this pull request Jan 7, 2021

[Modern] Deferred queries #2194

Open

robrichard force-pushed the spec-defer branch 2 times, most recently from 1d4dcfb to b828aeb Compare December 16, 2021 23:42

robrichard force-pushed the spec-defer branch from b828aeb to fb81711 Compare December 17, 2021 14:29

alunyov reviewed Dec 17, 2021

View reviewed changes

alunyov mentioned this pull request Dec 17, 2021

Allow Node interface id field to be configurable. #3638

Closed

robrichard force-pushed the spec-defer branch from fb81711 to 26ee9b4 Compare December 18, 2021 16:29

robrichard force-pushed the spec-defer branch from 26ee9b4 to 1726748 Compare January 27, 2022 19:12

robrichard added 4 commits January 28, 2022 09:52

Support hasNext on payloads

40f70ac

# Conflicts: # packages/relay-runtime/store/RelayModernQueryExecutor.js

Add DeferStreamInterface to conform to proposed graphql spec

09b54e0

update test fixtures

9ecd619

robrichard force-pushed the spec-defer branch from 1726748 to 9ecd619 Compare January 28, 2022 15:01

morrys mentioned this pull request Mar 28, 2023

[proposal] improve rendering for query without incremental data relay-tools/relay-hooks#241

Merged

robrichard closed this Oct 6, 2023

Uh oh!

Support defer/stream spec proposal #3121

Support defer/stream spec proposal #3121

Uh oh!

Conversation

robrichard commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josephsavona commented Jul 7, 2020

Uh oh!

robrichard commented Jul 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kassens Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephsavona left a comment

Choose a reason for hiding this comment

Uh oh!

robrichard commented Jul 14, 2020

Uh oh!

robrichard commented Nov 12, 2020

Uh oh!

sibelius commented Nov 12, 2020

Uh oh!

robrichard commented Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chriserickson commented Jun 17, 2021

Uh oh!

chriserickson commented Jun 17, 2021

Uh oh!

sorenhoyer commented Sep 2, 2021

Uh oh!

robrichard commented Dec 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Dec 20, 2021

Uh oh!

alunyov commented Dec 20, 2021

Uh oh!

Tiedye commented May 24, 2022

Uh oh!

robrichard commented Oct 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

robrichard commented Jul 7, 2020 •

edited

Loading

kassens Jul 30, 2020 •

edited

Loading

robrichard commented Dec 21, 2020 •

edited

Loading