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

ADD 'includeWsHeaders' property for GraphQL replication #4533

Merged
merged 3 commits into from
Mar 22, 2023

Conversation

johnnyvdberg
Copy link
Contributor

@johnnyvdberg johnnyvdberg commented Mar 9, 2023

This PR contains:

  • A NEW FEATURE

Describe the problem you have without this PR

I ran into a similar issue as #4113. Some GraphQL backends only support authenticating web sockets by setting an HTTP header. To circumvent the current behaviour, I implemented my own replication strategy, but with this small change we can get back to the official replication strategy.

Proposed use:

{
      ...
      headers: {
        Authorization: <Bearer token>,
      },
      pull: {
        ...
        includeWsHeaders: true,
      }
}

Todos

  • Tests
  • Documentation
  • Typings

Let me know what you think

@pubkey
Copy link
Owner

pubkey commented Mar 9, 2023

Code looks good. But we also need a test to ensure this will not break in the future.

@johnnyvdberg
Copy link
Contributor Author

Added a test, I struggled to get the tests running on an M1 machine, so could contain an error.

@pubkey
Copy link
Owner

pubkey commented Mar 9, 2023

This test only ensures that the headers exist on the client state, not that they reach the backend.
The test should reproduce your initial problem.

@pubkey
Copy link
Owner

pubkey commented Mar 13, 2023

@johnnyvdberg

@johnnyvdberg johnnyvdberg force-pushed the feature/add-ws-headers branch from d20b6bb to f0fbc6e Compare March 16, 2023 08:39
@johnnyvdberg johnnyvdberg changed the title ADD 'includeHttpHeadersWs' property for GraphQL replication ADD 'includeWsHeaders' property for GraphQL replication Mar 16, 2023
@johnnyvdberg
Copy link
Contributor Author

Finally got around to writing a proper test. I added a very basic auth check in the graphql-ws server that validates the headers like auth usage with token expiration, validation and refresh. Wasn't really happy with the naming, so I renamed the property as well.

@johnnyvdberg
Copy link
Contributor Author

I think the test might be a bit flaky, I wasn't able to reproduce the same error when testing locally.

@pubkey pubkey merged commit a347b11 into pubkey:master Mar 22, 2023
@pubkey
Copy link
Owner

pubkey commented Mar 22, 2023

Thank you, merged!

pubkey added a commit that referenced this pull request Mar 22, 2023
@johnnyvdberg johnnyvdberg deleted the feature/add-ws-headers branch March 29, 2023 08:48
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