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 AbortSignal support #2241

Open
fedyk opened this issue Dec 17, 2024 · 7 comments
Open

Add AbortSignal support #2241

fedyk opened this issue Dec 17, 2024 · 7 comments

Comments

@fedyk
Copy link

fedyk commented Dec 17, 2024

Is your feature request related to a problem? Please describe.

Since v14, Node.js supports AbortSignal and AbortController. Both APIs allows to control async requests.

It allows the implementation of complex flows. For instance, you have 30 seconds to make multiple requests to API. With one AbortSingnal.timeout() you can easily control the timing of the request:

const signal = AbortSingnal.timeout(30_000)
const invoice1 = await stripe.invoices.retrieve("inv_1", { signal })
const invoice2 = await stripe.invoices.retrieve("inv_1", { signal })
const customer1 = await stripe.customers.retrieve(invoice1.customer_id, { signal })
const customer2 = await stripe.customers.retrieve(invoice2.customer_id, { signal })

Describe the solution you'd like

Underhoos, stripe-node uses native fetch or http.request. Both methods already have built-in support for AbortSingal. By adding an option for passing signal in Stripe API methods, developers can leverage the signals in Stripe API.

Describe alternatives you've considered

No response

Additional context

No response

@fedyk
Copy link
Author

fedyk commented Dec 17, 2024

The merge with suggested changes
#2240

@jar-stripe
Copy link
Contributor

Hi @fedyk , thanks for filing the issue! The complex flow example is interesting, and not something I think I've considered before. Can you give me an example of how you'd use that ability to limit a complex flow to one timeout value ? I.e. is this a specific issue that you are trying to shore up in your implementation?

Also, the Node SDK has to support Node 12 and above. Is there a backfill for this for Node 12?

Thanks!

@fedyk
Copy link
Author

fedyk commented Dec 18, 2024

Can you give me an example of how you'd use that ability to limit a complex flow to one timeout value ?

I am working on endpoint that has 30s limit before the being cancelled. On timeout, the request is repeated, so previous API calls need to be stopped.

The endpoint calls multiple APIs over HTTP (including Stripe). The endpoint has one time "budget" to call all services. Its unknown how long each API call will take, timeout option make difficult to control all HTTP API calls in the endpoint. And signal makes this task trivial 🙂

Also, the Node SDK has to support Node 12 and above. Is there a backfill for this for Node 12?

For node v12, stripe-node uses http.request which supports signal parameter since v14. As result, the signal option will be ignored.

EOL for node v12 is 2022-04-30. I'd consider of adding a deprecation warning and ask to upgrade the node.js version.

@jar-stripe
Copy link
Contributor

jar-stripe commented Dec 18, 2024

Thanks for the additional info @fedyk ; unfortunately, retiring older versions of runtimes is more difficult than it would seem for us, because of our wide install base. We are going to revisit older platform support in the coming year though!

That being said though, I don't think we can support a feature that only works as documented on certain supported platforms, so we may need to hold off on this PR until we can retire Node v12. I will confirm with my team, but in the mean time, you could wrap fetch with a function that lets you inject your AbortSignal, and then use createFetchHttpClient when creating your String object to flow requests though your new function. Something like:

let globalSignal = null;
const fetchWithSignal = (
  input: string | URL | globalThis.Request,
  init?: RequestInit,
): Promise<Response> => {
  if (!init) {
    init = {};
  }
  return fetch(input, {
    ...init,
    signal: globalSignal,
  });
};
const StripeClient = new Stripe('API_KEY', {
  httpClient: Stripe.createFetchHttpClient(fetchWithSignal),
});
...
globalSignal =  AbortSignal.timeout(30_000);
const invoice1 = await stripe.invoices.retrieve("inv_1")
const invoice2 = await stripe.invoices.retrieve("inv_1")
const customer1 = await stripe.customers.retrieve(invoice1.customer_id)
const customer2 = await stripe.customers.retrieve(invoice2.customer_id)

Would something like this work for you here?

@fedyk
Copy link
Author

fedyk commented Dec 19, 2024

Thanks for suggestion, we will consider your solution 👍

@jar-stripe
Copy link
Contributor

Sounds good. I'll keep this issue open for now but please let me know if this works for you!

@fedyk
Copy link
Author

fedyk commented Dec 24, 2024

We created our own Stripe API client. We used native fetch with signal support. We need only few methods, so this solution works fine for us.

.Thanks for the help anyway!

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

No branches or pull requests

2 participants