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

Align @defer / @stream compiler assumptions to GraphQL spec #4159

Conversation

tobias-tengler
Copy link
Contributor

@tobias-tengler tobias-tengler commented Dec 28, 2022

Changes

@stream

  • Rename initial_count argument to initialCount
  • Allow initial_count argument to be not defined, since the official directive definition specifies it as nullable and with a default value of 0
  • Validate initialCount argument being a non-negative int, if it's defined
  • Make if argument non-null

@defer

  • Make if argument non-null

Open questions

  • Should I also change the shape of @stream_connection? I haven't touched its signature yet, since it's not part of the GraphQL specification.
  • Some of the changes in this PR are breaking changes. @stream / @defer are experimental features in OSS, so it shouldn't be a problem to just ship these changes. To my knowledge, Meta should be using these features in production though. Is this a blocker for these breaking changes?

Closes #4158

@tobias-tengler tobias-tengler changed the title Tte/stream defer directive definitions Align @defer / @stream compiler assumptions to GraphQL spec Dec 28, 2022
@tobias-tengler tobias-tengler marked this pull request as ready for review December 28, 2022 18:26
Copy link
Contributor

@alunyov alunyov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this!

I think @rbalicki2 were also looking into this at some point (so maybe have more context).

Specifically for this PR - I don't think we will be able to land that internally. So we need to figure out the strategy on how to make this work.

Some high-level observations:

  1. We need to be able to minimize the changes in this diff to reduce the risk of breaking things internally.

  2. For the @defer/@stream we need to somehow express to the compiler what type of directive is expected. Maybe via config, I think we have schema_config section in the compiler config maybe put it there. We can add an explicit version - something like what type of arguments is expected, or even SDL for these directives, or maybe have something like enum DEFER_STREAM_MODE = CustomRelay | OSS

@tobias-tengler
Copy link
Contributor Author

I also just watched the GraphQL spec meetings of December and there seems to be once more discussion around the shape of the response. In particular the necessity of the label on the directives is being put to question. It's also proposed to no longer include the path in each incremental patch. So I would anticipate that Relay might has to do some more changes to how @defer / @stream are handled in OSS. So having a config option might be the best thing to support Meta and OSS side-by-side.

@Tiedye
Copy link

Tiedye commented May 17, 2023

Any updates?

@tobias-tengler
Copy link
Contributor Author

I can have another go at this in the coming weeks. I'll try to add a config option and keep the default as the previous behavior so all the test changes and so on can be reverted again and we have a smaller diff to land

@tobias-tengler tobias-tengler marked this pull request as draft August 31, 2023 19:01
@tobias-tengler
Copy link
Contributor Author

Closing this in favor of #4467

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

Successfully merging this pull request may close these issues.

Align @defer and @stream with current draft specification
5 participants