Re: [whatwg/dom] Specify that AbortController#abort() be bound to AbortController instance (#981)

> Making the suggested change would allow for this code, instead...
> 
> ```js
> const {signal, abort} = new AbortController();
> 
> // delegate abort
> $('#someButton').addEventListener('click', abort);
> ```

This is an antipattern. It means abort is being called with an `Event` object. `controller.abort()` currently accepts zero arguments, but that might change in future. For example, it might become `controller.abort(reason)`, and that might cause an unexpected change to code like above. I wrote about this antipattern here https://jakearchibald.com/2021/function-callback-risks/.

As for the revealing constructor pattern, I don't think it works outside of the most basic code examples.

Let's make the "abort button" example a bit more realistic:

```js
async function fetchAndDisplay() {
  const controller = new AbortController();
  const abortClick = () => controller.abort();
  stopButton.addEventListener('click', abortClick);

  try {
    const response = await fetch(url, { signal: controller.signal });
    await displayResult(response);
  } finally {
    stopButton.removeEventListener('click', abortClick);
  }
}
```

To avoid leaks, the abort button listener is removed on job completion, failure, or abort. Now let's try revealing constructor:

```js
async function fetchAndDisplay() {
  const signal = new AbortSignal(abort => {
    stopButton.addEventListener('click', () => abort());
  });

  try {
    const response = await fetch(url, { signal });
    await displayResult(response);
  } finally {
    stopButton.removeEventListener('click', /* ah bollocks */);
  }
}
```

We can't remove the listener in this case, because it isn't in scope. You can work around this by complicating the code:

```js
function fetchAndDisplay() {
  const job = Promise.resolve().then(async () => {
    const response = await fetch(url, { signal });
    await displayResult(response);
  });

  const signal = new AbortSignal(abort => {
    const abortClick = () => abort(); 
    stopButton.addEventListener('click', abortClick);
    job.finally(() => stopButton.removeEventListener('click', abortClick));
  });

  return job;
}
```

This maintains the revealing constructor pattern, but now the code is split between two places, rather than having a linear flow. There are also crossed dependencies; the 'job' needs the signal to pass to fetch, but the signal needs the 'job' to know when the job settles. This is why the job needs to use `Promise.resolve()` to wait for a microtask.

Another way to solve this is to break out of the revealing constructor:

```js
async function fetchAndDisplay() {
  let abortClick;
  const signal = new AbortSignal(abort => {
    abortClick = () => abort();
    stopButton.addEventListener('click', abortClick);
  });

  try {
    const response = await fetch(url, { signal });
    await displayResult(response);
  } finally {
    stopButton.removeEventListener('click', abortClick);
  }
}
```

But it doesn't feel like we're getting much benefit over the API we have now.

An abort button is the simple case. Another common pattern is "abort the current operation in favour of a new operation". With the current API:

```js
let currentJob = Promise.resolve();
let currentController;

async function showSearchResults(input) {
  if (currentController) currentController.abort();
  currentController = new AbortController();

  return currentJob = currentJob.catch(() => {}).then(async () => {
    const response = await fetch(getSearchUrl(input), { signal: currentController.signal });
    await displayResult(response);
  });
}
```

I can't think of a way to make this fit the revealing constructor pattern, so again I find myself just passing it outside of the constructor:

```js
let currentJob = Promise.resolve();
let currentAbort;

async function showSearchResults(input) {
  if (currentAbort) currentAbort();
  const signal = new AbortSignal(a => currentAbort = a);

  return currentJob = currentJob.catch(() => {}).then(async () => {
    const response = await fetch(getSearchUrl(input), { signal });
    await displayResult(response);
  });
}
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/dom/issues/981#issuecomment-845048934