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

Extend HTTPRequest to support path overrides #288

Merged
merged 4 commits into from
Oct 3, 2024
Merged

Conversation

ecooper
Copy link
Contributor

@ecooper ecooper commented Oct 2, 2024

This PR extends the existing HTTPRequest to take an optional path to support POST requests to endpoints other than /query/1.

Description

We'll need to support requests to Fauna APIs beyond /query/1. Today, this API path is hardcoded in both the fetch and http2 client wrappers. This PR does the following:

  • Extends HTTPRequest to accept an optional path
  • Updates FetchClient to use default API paths, but override per request when one is provided on HTTPRequest
  • Updates NodeHTTP2Client to use default API paths, but override per request when one is provided on HTTPRequest
  • Adds a readonly FaunaAPI to represent the supported API paths in the driver
  • Restricts HTTPRequest["path"] to the union of supported endpoints via FaunaAPI

Motivation and context

We need to support more endpoints in this driver. The current http clients really only have a fixed concept of requesting queries or streaming. This change allows for a backwards compatible way to introduce providing a path without breaking existing code or interfaces.

Alternatives considered:

  • Making HTTPClient.request be (path: SupportedFaunaAPIs, req: HTTPRequest): backwards incompat
  • Supporting both the signature above and the current one: Seems overly complex
  • Adding path to a second optional argument of a new HTTPRequestOptions interface to request: Best alternative, but also lacking much more substance--especially since HTTPRequest already supports additional meta options, like timeout.

In the future, we may want to reconsider redesigning these interfaces in a major version bump, but that is out of scope for this PR.

Definitely open the other implementation approaches!

How was the change tested?

These changes have dedicated unit tests. The unit tests validate the override logic as well as type enforcement.

Because these changes are backwards compatible, all existing code and tests should work as expected.

Screenshots (if appropriate):

N/A

Change types

    • Bug fix (non-breaking change that fixes an issue)
    • New feature (non-breaking change that adds functionality)
    • Breaking change (backwards-incompatible fix or feature)

Checklist:

    • My code follows the code style of this project.
    • My change requires a change to Fauna documentation.
    • My change requires a change to the README, and I have updated it accordingly.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

@echo-bravo-yahoo echo-bravo-yahoo left a comment

Choose a reason for hiding this comment

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

i'm ok with this path forward (and with a major version bump that moves these to obj args as well).

how much blast radius is there in the docs site if we make a breaking change in these?

__tests__/unit/fetch-client.test.ts Show resolved Hide resolved
src/http-client/paths.ts Outdated Show resolved Hide resolved
@ecooper
Copy link
Contributor Author

ecooper commented Oct 2, 2024

i'm ok with this path forward (and with a major version bump that moves these to obj args as well).

how much blast radius is there in the docs site if we make a breaking change in these?

Because these are largely internals they are really only covered by the docs that are auto-published here: https://fauna.github.io/fauna-js/2.2.0/

@ecooper ecooper merged commit dbf5646 into main Oct 3, 2024
5 checks passed
@ecooper ecooper deleted the FE-5921-extend-client branch October 3, 2024 16:35
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.

4 participants