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

WIP - imported from other module #10

Closed
wants to merge 8 commits into from
Closed

WIP - imported from other module #10

wants to merge 8 commits into from

Conversation

leftieFriele
Copy link
Contributor

@leftieFriele leftieFriele commented Oct 29, 2024

WIP - Creating an improved HTTP client which has a circuit breaker.

Note, these are just notes 😄

Todos

  • identify the metrics on client which we need to continue having
  • add metrics support
  • add prometheus hook into opossum
  • close the gap on features in client and this one.... not sure what those are yet.

Open questions ❓

  • request which we are using here is single origin, which means that a request to https://google.com returning a location header with https://www.google.com will not redirect as it's a different host. the client implementation supports redirectable and it's used in the resolvers
  • decide on default redirect behaviour
  • decide on options surface, how much control should there be?
  • should we rename this branch to alpha to create a release?
  • do we add retries to the client?

Errors

If options.fallback is not set, we wrap errors in a HttpClientError class.

const client = new HttpClient();
try {
  await client.request(...)
} catch (err) {
 if (err instance of HttpClientError) {
  // do stuff
 }
}

Constructor options

abortController

we need to be able to cancel requests in opossum

followRedirects

if we should follow redirects or not, default behaviour is to not follow (???)

fallback

const client = new HttpClient({ fallback: function () { return 'We are down.' } });
// can also be set like this

client.fallback(function () { 'We are down'; });

Closes #11

@leftieFriele leftieFriele self-assigned this Oct 29, 2024
@leftieFriele leftieFriele added this to the first-beta milestone Oct 29, 2024
@leftieFriele leftieFriele force-pushed the first-beta branch 2 times, most recently from da7e61a to 8c274ea Compare October 29, 2024 09:28
Copy link
Member

@digitalsadhu digitalsadhu left a comment

Choose a reason for hiding this comment

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

So far, so good I reckon.

@@ -38,13 +42,21 @@ export default class PodiumHttpClient {
timeout = 500,
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this timeout should be 1000. Wonder why @trygve-lie went for 500 🤔

lib/http-client.js Outdated Show resolved Hide resolved
@leftieFriele leftieFriele added the enhancement New feature or request label Oct 30, 2024
@digitalsadhu
Copy link
Member

I think for the Podium client, fallback as a string is more useful than fallback as a function.

@leftieFriele leftieFriele deleted the first-beta branch October 31, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement a first beta to replace the existing podium http client
2 participants