Don't use functions as callbacks unless they're designed for it

Here's an old pattern that seems to be making a comeback:

// Convert some numbers into human-readable strings:
import { toReadableNumber } from 'some-library';
const readableNumbers = someNumbers.map(toReadableNumber);

Where the implementation of toReadableNumber is like this:

export function toReadableNumber(num) {
  // Return num as string in a human readable form.
  // Eg 10000000 might become '10,000,000'
}

Everything works great until some-library is updated, then everything breaks. But it isn't some-library's fault – they never designed toReadableNumber to be a callback to array.map.

Here's the problem:

// We think of:
const readableNumbers = someNumbers.map(toReadableNumber);
// …as being like:
const readableNumbers = someNumbers.map((n) => toReadableNumber(n));
// …but it's more like:
const readableNumbers = someNumbers.map((item, index, arr) =>
  toReadableNumber(item, index, arr),
);

We're also passing the index of the item in the array, and the array itself to toReadableNumber. This worked fine at first, because toReadableNumber only had one parameter, but in the new version:

export function toReadableNumber(num, base = 10) {
  // Return num as string in a human readable form.
  // In base 10 by default, but this can be changed.
}

The developers of toReadableNumber felt they were making a backwards-compatible change. They added a new parameter, and gave it a default value. However, they didn't expect that some code would have already been calling the function with three arguments.

toReadableNumber wasn't designed to be a callback to array.map, so the safe thing to do is create your own function that is designed to work with array.map:

const readableNumbers = someNumbers.map((n) => toReadableNumber(n));

And that's it! The developers of toReadableNumber can now add parameters without breaking our code.

The same issue, but with web platform functions

Here's an example I saw recently:

// A promise for the next frame:
const nextFrame = () => new Promise(requestAnimationFrame);

But this is equivalent to:

const nextFrame = () =>
  new Promise((resolve, reject) => requestAnimationFrame(resolve, reject));

This works today because requestAnimationFrame only does something with the first argument, but that might not be true forever. A extra parameter might be added, and the above code could break in whatever browser ships the updated requestAnimationFrame.

The best example of this pattern going wrong is probably:

const parsedInts = ['-10', '0', '10', '20', '30'].map(parseInt);

If anyone asks you the result of that in a tech interview, I recommend rolling your eyes and walking out. But anyway, the answer is [-10, NaN, 2, 6, 12], because parseInt has a second parameter.

Option objects can have the same gotcha

Chrome 90 will allow you to use an AbortSignal to remove an event listener, meaning a single AbortSignal can be used to remove event listeners, cancel fetches, and anything else that supports signals:

const controller = new AbortController();
const { signal } = controller;

el.addEventListener('mousemove', callback, { signal });
el.addEventListener('pointermove', callback, { signal });
el.addEventListener('touchmove', callback, { signal });

// Later, remove all three listeners:
controller.abort();

However, I saw an example where instead of:

const controller = new AbortController();
const { signal } = controller;
el.addEventListener(name, callback, { signal });

…they did this:

const controller = new AbortController();
el.addEventListener(name, callback, controller);

As with the callback examples, this works today, but it might break in future.

An AbortController was not designed to be an option object to addEventListener. It works right now because the only thing AbortController and the addEventListener options have in common is the signal property.

If, say in future, AbortController gets a controller.capture(otherController) method, the behaviour of your listeners will change, because addEventListener will see a truthy value in the capture property, and capture is a valid option for addEventListener.

As with the callback examples, it's best to create an object that's designed to be addEventListener options:

const controller = new AbortController();
const options = { signal: controller.signal };
el.addEventListener(name, callback, options);
// Although I find this pattern easier when multiple things
// get the same signal:
const { signal } = controller;
el.addEventListener(name, callback, { signal });

And that's it! Watch out for functions being used as callbacks, and objects being used as options, unless they were designed for those purposes. Unfortunately I'm not aware of a linting rule that catches it (edit: looks like there's a rule that catches some cases, thanks James Ross!).

TypeScript doesn't solve this

Edit: When I first posted this, it had a little note at the end showing that TypeScript doesn't prevent this issue, but I still got folks on Twitter telling me to "just use TypeScript", so let's look at it in more detail.

TypeScript doesn't like this:

function oneArg(arg1: string) {
  console.log(arg1);
}

oneArg('hello', 'world');
//              ^^^^^^^
// Expected 1 arguments, but got 2.

But it's fine with this:

function twoArgCallback(cb: (arg1: string, arg2: string) => void) {
  cb('hello', 'world');
}

twoArgCallback(oneArg);

…even though the result is the same.

Therefore TypeScript is fine with this:

function toReadableNumber(num): string {
  // Return num as string in a human readable form.
  // Eg 10000000 might become '10,000,000'
  return '';
}

const readableNumbers = [1, 2, 3].map(toReadableNumber);

If toReadableNumber changed to add a second string param, TypeScript would complain, but that isn't what happened in the example. An additional number param was added, and this meets the type constraints.

Things get worse with the requestAnimationFrame example, because this goes wrong after a new version of a browser is deployed, not when a new version of your project is deployed. Additionally, TypeScript DOM types tend to lag behind what browsers ship by months.

I'm a fan of TypeScript, this blog is built using TypeScript, but it does not solve this problem, and probably shouldn't.

Most other typed languages behave differently to TypeScript here, and disallow casting callbacks in this way, but TypeScript's behaviour here is intentional, otherwise it'd reject the following:

const numbers = [1, 2, 3];
const doubledNumbers = numbers.map((n) => n * 2);

…since the callback given to map is cast to a callback with more params. The above is an extremely common pattern in JavaScript, and totally safe, so it's understandable that TypeScript made an exception for it.

The question is "is the function intended to be a callback to map?", and in a JavaScript world that isn't really solvable with types. Instead, I wonder if new JS and web functions should throw if they're called with too many arguments. This would 'reserve' additional parameters for future use. We couldn't do it with existing functions, as that would break backwards compatibility, but we could show console warnings for existing functions that we might want to add parameters to later. I proposed this idea, but folks don't seem too excited about it 😀.

Things are a bit tougher when it comes to option objects:

interface Options {
  reverse?: boolean;
}

function whatever({ reverse = false }: Options = {}) {
  console.log(reverse);
}

You could say that APIs should warn/fail if the object passed to whatever has properties other than reverse. But in this example:

whatever({ reverse: true });

…we're passing an object with properties like toString, constructor, valueOf, hasOwnProperty etc etc since the object above is an instance of Object. It seems too restrictive to require that the properties are 'own' properties (that isn't how it currently works at runtime), but maybe we could add some allowance for properties that come with Object.

Thanks to my podcast husband Surma for proof-reading and suggestions, and Ryan Cavanaugh for correcting me on the TypeScript stuff.

View this page on GitHub

Comments powered by Disqus

Jake Archibald next to a 90km sign

Hello, I’m Jake and that is my tired face. I’m a developer of sorts.

Elsewhere

Contact

Feel free to throw me an email, unless you're a recruiter, or someone trying to offer me 'sponsored content' for this site, in which case write your request on a piece of paper, and fling it out the window.