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

feat(client): specify headers for individual subscribe calls #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RMHonor
Copy link

@RMHonor RMHonor commented Nov 21, 2024

This allows each subscribe call on the client to specify request headers, useful if your requests need to send different headers depending on the operation.

It will merge the headers with any specified on the client, overriding any duplicates.

Note: this only applies to distinct connection mode, as single connection won't be able to send new headers per oepration as there's only one request.

Example, pulling the Apollo context headers to match the HttpLink behaviour:

export class SSELink extends ApolloLink {
  private client: Client;

  constructor(options: ClientOptions) {
    super();
    this.client = createClient(options);
  }

  public request(operation: Operation): Observable<FetchResult> {
    return new Observable((sink) => {
      return this.client.subscribe<FetchResult>(
        { ...operation, query: print(operation.query) },
        {
          next: sink.next.bind(sink),
          complete: sink.complete.bind(sink),
          error: sink.error.bind(sink),
        },
        undefined,
        operation.getContext().headers,
      );
    });
  }
}

@RMHonor RMHonor marked this pull request as ready for review November 21, 2024 10:40
Copy link
Owner

@enisdenjo enisdenjo 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 this! Certnaily something we could improve.

By convention, I do things the other way around - instead of adding an extra argument to the subscribe and thus fixing the 3rd argument to always be headers (where you could potentially want to change other things, like the url or retry behaviour or any other related config option); I'd rather add an extra argument to headers instead which accepts the RequestParams and changes headers accordingly.

Not only would you remove the lock on the subscribe method; but, since there's a high chance multiple subscribes will need to change headers, you would also centralise the headers manipulation (instead of changing multiple subscribes you would change one headers).

What do you think?

@RMHonor
Copy link
Author

RMHonor commented Nov 26, 2024

That was my first instinct as well, but the RequestParams type purely contains GQL information, and by exposing this on the subscribe call, it allows any sort of manipulation at the point of request. In my example case it allows us access to the Apollo client context so we can match the HttpLink behvaiour.

However I recognise that we probably shouldn't be designing an API around alternative systems or specific use cases.

An alternative idea, could the requestParams take arbitrary data (a bit like extensions but not sent on the network), which would then be accessible in the headers callback? I think this follows patterns I've seen elsewhere.

// extend the request params type
interface RequestParams<T = any> {
  operationName?: string;
  query: string;
  variables?: Record<string, unknown>;
  extensions?: Record<string, unknown>;
  extra?: T;
}

// extra accessible from headers
const client = createClient({
  uri: "https://theguild.com/graphql",
  headers: (requestParams) => ({
    "x-my-header": requestParams.extra,
  }),
});

// call subscribe with arbitrary params
client.subscribe<ExecutionResult>(
  { ...operation, query: print(operation.query), extra: "foo" },
  {
   next: sink.next.bind(sink),
   complete: sink.complete.bind(sink),
   error: sink.error.bind(sink),
 },
  undefined,
  operation.getContext().headers,
);

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

Successfully merging this pull request may close these issues.

2 participants