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

RFC (core): Update Incremental Delivery response payload support #3003

Closed
kitten opened this issue Mar 6, 2023 · 0 comments · Fixed by #3007
Closed

RFC (core): Update Incremental Delivery response payload support #3003

kitten opened this issue Mar 6, 2023 · 0 comments · Fixed by #3007
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release

Comments

@kitten
Copy link
Member

kitten commented Mar 6, 2023

This RFC supersedes #2995.

Summary

See spec edits for incremental delivery support in graphql-spec: https://github.com/graphql/graphql-spec/pull/742/files#diff-98d0cd153b72b63c417ad4238e8cc0d3385691ccbde7f7674bc0d2a718b896ec
See updated implementation of graphql-js: graphql/graphql-js#3659

Previously, our current definition of ExecutionResult looks like the following:

export type ExecutionResult =
| {
errors?:
| Array<Partial<GraphQLError> | string | Error>
| readonly GraphQLError[];
data?: null | Record<string, any>;
extensions?: Record<string, any>;
hasNext?: boolean;
}
| {
errors?:
| Array<Partial<GraphQLError> | string | Error>
| readonly GraphQLError[];
data: any;
path: (string | number)[];
hasNext?: boolean;
};

export type ExecutionResult =
  | {
      errors?:
        | Array<Partial<GraphQLError> | string | Error>
        | readonly GraphQLError[];
      data?: null | Record<string, any>;
      extensions?: Record<string, any>;
      hasNext?: boolean;
    }
  | {
      errors?:
        | Array<Partial<GraphQLError> | string | Error>
        | readonly GraphQLError[];
      data: any;
      path: (string | number)[];
      hasNext?: boolean;
    };

The spec has been updated to instead look like the following for incremental responses:

interface BaseExecutionResult {
  errors?:
    | Array<Partial<GraphQLError> | string | Error>
    | readonly GraphQLError[];
  extensions?: Record<string, any>;
}

interface BaseIncrementalDeliveryResult {
  label?: string;
  path: (string | number)[];
}

interface DeferResult extends BaseIncrementalDeliveryResult {
  data: unknown | null
}

interface StreamResult extends BaseIncrementalDeliveryResult {
  items: unknown[] | null
}

type IncrementalResult = DeferResult | StreamResult;

export interface ExecutionResult extends BaseExecutionResult {
  incremental?: IncrementalDeliveryResult[];
  data?: null | Record<string, any>;
  hasNext?: boolean;
}

Proposed Solution

  • Update @urql/core with above types (with modifications?)
    • We probably don't want to support the old format, since it was experimental. Instead, I'd flag this as a minor with a note in the changelog that reflects why we've done this.
  • Update mergeResultPatch to reflect the new format:
    export const mergeResultPatch = (
    prevResult: OperationResult,
    patch: ExecutionResult,
    response?: any
    ): OperationResult => {

Requirements

  • We must be able to comply with the new format in full; again, label is treated as optional and we don't currently use its information
  • We should look into how extensions and errors are now treated, since there could be multiple ones sent per response.
  • We should check what happens to result.extensions and result.errors if those can also be present on result.incremental[].extensions and result.incremental[].errors now
@kitten kitten added the future 🔮 An enhancement or feature proposal that will be addressed after the next release label Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant