feat(web-api): add support for AbortSignal

[LinusU]

Summary

This adds support for aborting in-flight requests, using the standardized AbortSignal

Fixes #1761

I've designed this as an additional parameter instead of mixing it with the body of the specific endpoint. I believe that this is a sounder approach as there will never be any conflicts, and it also avoids the current problem of token also being present in the request body as well as in the header when specified this way.

Requirements (place an x in each [ ])

• I've read and understood the Contributing Guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.

[salesforce-cla]

Thanks for the contribution! Before we can merge this, we need @LinusU to sign the Salesforce Inc. Contributor License Agreement.

[LinusU]

@salesforce-cla signed ✅

[zimeg]

Hey @LinusU! 👋 Thanks so much for sending this contribution in 👾 ✨

At the moment we're exploring another approach of a custom fetch attribute as an option to the WebClient alongside efforts of #2359.

I don't have more to share for @slack/web-api at the moment, but an example from @slack/webhook might resemble:

node-slack-sdk/packages/webhook/README.md

Lines 95 to 133 in 56a6183

	import { IncomingWebhook } from "@slack/webhook";
	import { ProxyAgent, fetch as undiciFetch } from "undici";
	const url = process.env.SLACK_WEBHOOK_URL;
	/**
	* Configure your proxy agent here
	* @see {@link https://undici.nodejs.org/#/docs/api/ProxyAgent.md}
	*/
	const proxyAgent = new ProxyAgent({
	uri: new URL("http://localhost:8888"),
	proxyTls: {
	signal: AbortSignal.timeout(400),
	},
	});
	/**
	* Implement a custom fetch adapter
	* @type {typeof globalThis.fetch}
	* @see {@link https://undici.nodejs.org/#/docs/api/Fetch.md}
	*/
	const myFetch = async (url, opts) => {
	return await undiciFetch(url, {
	...opts,
	dispatcher: proxyAgent,
	});
	};
	// Initialize with the custom fetch adapater and proxy
	const webhook = new IncomingWebhook(url, {
	fetch: myFetch,
	});
	// Sending this webhook will now go through the proxy
	(async () => {
	await webhook.send({
	text: "I've got news for you...",
	});
	})();

Let's keep this open while that's explored, but I'm curious if the fetch approach might also be suitable for you?

[LinusU]

Hey @zimeg, thanks for taking a look at this! 👋

[...] but I'm curious if the fetch approach might also be suitable for you?

I would love for this package to be based on fetch instead of Aaxios, and I took special care when writing this to not expose any Axios-specific behaviour (e.g. that is why I do the little dance to throw the correct error, instead of the Axios-specific cancelled error). So this should pair up great together with the fetch move!

With that said, I do not think that being able to pass in a custom fetch-function would solve the problem here. We are using AbortSignals across large parts of our app, not just as a tool to set a specific timeout on http requests.

I need to be able to pass signal individually to each api call, not at the creation of WebClient.

---

Side note: the example you linked will cancel every single request made 400 milliseconds after the ProxyAgent was constructed, not after each request has ran for 400ms?

[LinusU]

ping @zimeg, have you had a chance to read my last comment? 🙏

[LinusU]

ping @zimeg, have you had a chance to look at this? 🙏

[zimeg]

@LinusU Thank you for keeping these changes alive and relevant 💌

I haven't had a good chance to test this yet but I do now understand it's a separate change from what I suggest earlier and think your approach of adding another argument is most solid:

  public async apiCall(
    method: string,
    options: Record<string, unknown> = {},
+   config?: { signal?: AbortSignal },
  ): Promise<WebAPICallResult> {

Leaving a few comments from code review which is looking great! I do notice a merge conflict appeared and I left a few questions for this meantime 🚢

[zimeg]

🔬 question: Is CanceledError the most common error code here? I understand we can catch related errors in following changes, but I notice separate codes on MDN docs:

• AbortError
• TimeoutError

And it's not obvious to me if that's exhaustive or if it's missing CanceledError and more! 🤓

[LinusU]

CanceledError here is from axios, so it will always be CanceledError on every platform, since it's an error constructed in "user space" code.

As long as axios is used, this will work. And as I said earlier, this intentionally does not expose CancelError, so that a switch to fetch would be seamless! 🚀

[zimeg]

🎁 thought: We throw new AbortError above and I'm uncertain if we want to use new here or continue referencing the pRetry package? The tests above do show this is caught as expected which is great.

[LinusU]

I've removed the pRetry. part now, to make it match the rest of the code. However, it's the same class, just exposed in two different places, so no runtime difference ☺️

[changeset-bot]

🦋 Changeset detected

Latest commit: 83afbff

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@slack/web-api	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[LinusU]

Sorry for the super late reply 🙈 I totally missed that you answered me the same day!

I've rebased on latest main, and also changed pRetry.AbortError -> AbortError

Tests are still passing locally for me, and I would still love to see this merged! 🙏