Don’t use functions as callbacks unless they’re designed for it
Here’s an old model that seems to be making a comeback:
// Convert some numbers into human-readable strings
import { toReadableNumber } from 'some-library';
const readableNumbers = someNumbers.map(toReadableNumber);
Copy the codeToReadableNumber is implemented as follows:
// some-library
export function toReadableNumber(num) {
// Return num as a readable string
// For example 10000000 May become '10,000,000'
}
Copy the codeEverything was fine until the some-Library external library was updated, but after it was updated the above code broke. This is not some-Library’s fault, however, because the library never intended toReadableNumber to be used as a callback to array.map.
Here’s the problem:
// We consider the following code
const readableNumbers = someNumbers.map(toReadableNumber);
// Equivalent to:
const readableNumbers = someNumbers.map((n) = > toReadableNumber(n));
// But it actually looks like this:
const readableNumbers = someNumbers.map((item, index, arr) = >
toReadableNumber(item, index, arr),
);
Copy the codeWe also pass the index and the array itself to The toReadableNumber. This was fine at first because toReadableNumber had only one parameter, but in the new version it added one more:
export function toReadableNumber(num, base = 10) {
// Return num as a readable string
// The default base is 10, but the base can be changed
}
Copy the codeThe developers of toReadableNumber felt they had made a backward compatible change. They add a new parameter and assign it an initial value. But they didn’t realize that some code was already calling this function with three arguments.
ToReadableNumber is not designed to call array.map as a callback, so it’s safe to write a function of your own to array.map:
// In the arrow function we wrote, we explicitly pass num only
const readableNumbers = someNumbers.map((n) = > toReadableNumber(n));
Copy the codeThat’s it! ToReadableNumber developers can now add additional parameters without messing up our code.
The same problem applies to Web platform functions
Here’s an example I saw recently:
// Create a Promise object representing the next frame:
const nextFrame = () = > new Promise(requestAnimationFrame);
Copy the codeBut it’s the same as:
const nextFrame = () = >
new Promise((resolve, reject) = > requestAnimationFrame(resolve, reject));
Copy the codeThis code now works because requestAnimationFrame currently accepts only one parameter. But it is not certain that the future will accept only one parameter. This function may have an extra parameter in the future, so the above code may crash when the browser releases a new version of requestAnimationFrame. (For example, assume that the second argument is called reject in execution.)
The following example illustrates the problem with this pattern most clearly:
const parsedInts = ['- 10'.'0'.'10'.'20'.'30'].map(parseInt);
Copy the codeIf anyone asks you during a technical interview how this code works, give them a blank look and end the interview. But anyway, the answer is [-10, NaN, 2, 6, 12] because parseInt takes a second parameter. And the radix we pass in each time is 0, 1, 2, 3, 4.
Option objects can have the same problem
Chrome 90 will support AbortSignal to remove event listeners, which means AbortSignal can be used to remove event listeners, cancel requests, and anything else that supports signal.
const controller = new AbortController();
const { signal } = controller;
el.addEventListener('mousemove', callback, { signal });
el.addEventListener('pointermove', callback, { signal });
el.addEventListener('touchmove', callback, { signal });
// At some point after that, remove all three listeners:
controller.abort();
Copy the codeHowever, I saw an example that didn’t pass the option object like this:
const controller = new AbortController();
const { signal } = controller;
el.addEventListener(name, callback, { signal });
Copy the codeIt goes like this:
const controller = new AbortController();
el.addEventListener(name, callback, controller);
Copy the codeAs with the previous callback example, this approach may work now, but it may fail in the future.
An instance of AbortController is not designed as an option object for addEventListener. The above method now works only because the AbortController and addEventListener options have the same signal property.
If, say, AbortController later adds a controller.capture(otherController) method, then your listener’s behavior will change. Because addEventListener understands the capture attribute as a true value, and capture is a valid option for addEventListener.
As in the previous callback example, it is best to create an object that is specifically used as the addEventListener option.
const controller = new AbortController();
const options = { signal: controller.signal };
el.addEventListener(name, callback, options);
// However, the following pattern is more convenient when you need to reuse signal:
const { signal } = controller;
el.addEventListener(name, callback, { signal });
Copy the codeThat’s it! Be careful what functions are used as callbacks and what objects are used as options, unless they are made for that purpose. Unfortunately, I didn’t find a linting rule to find this pattern. (Update: There seems to be a rule for spotting this pattern in some cases, thanks James Ross!)
TypeScript doesn’t solve this problem
Update: When I first posted this post, I included a brief comment at the end showing why TypeScript doesn’t prevent this problem, but I still get a lot of “just use TypeScript” comments on Twitter, so let’s take a closer look at the issue.
TypeScript doesn’t like code that:
function oneArg(arg1: string) {
console.log(arg1);
}
oneArg('hello'.'world');
/ / ^ ^ ^ ^ ^ ^ ^
// Expected 1 arguments, but got 2.
Copy the codeBut it accepts this:
function twoArgCallback(cb: (arg1: string, arg2: string) = >void) {
cb('hello'.'world');
}
twoArgCallback(oneArg);
Copy the codeBut the result is the same, passing oneArg two parameters.
So TypeScript accepts the following code:
function toReadableNumber(num) :string {
// Return num as a readable string
// For example 10000000 May become '10,000,000'
return ' ';
}
const readableNumbers = [1.2.3].map(toReadableNumber);
Copy the codeTypeScript reports an error if toReadableNumber is modified to add a second string parameter, but that’s not what’s happening in this example. We added an additional parameter of type number, which satisfies the type constraint.
If it’s an example like the requestAnimationFrame example above, things get even worse. Because problems will show up after a new version of the browser is released, not when your project goes live. In addition, TypeScript DOM types tend to be months behind in changes to browser publishing.
I’m a fan of TypeScript, and this blog is built in TypeScript, but it doesn’t solve this problem, and probably shouldn’t.
Most other typed languages don’t behave the same way TypeScript does in this case, and they forbid casting callback types in this way, but TypeScript’s behavior here is intentional, otherwise it would reject the following code:
const numbers = [1.2.3];
const doubledNumbers = numbers.map((n) = > n * 2);
Copy the codeBecause the callback passed to map is converted to a callback with more parameters. This behavior is an extremely common pattern in JavaScript and is completely safe, so it makes sense that TypeScript makes an exception for this behavior.
The question here is “is this function used as a map callback?” This is not a problem that can be solved with typing in the JavaScript world. Instead, I wonder if new JS and Web functions should report errors when they are called with too many arguments. This will “reserve” some additional parameter locations for future use. You can’t add this behavior for existing functions because it would break backward compatibility, but you can add console warnings for existing functions that might add parameters in the future. I suggested the idea, but people were not particularly enthusiastic about the idea 😀.
It gets trickier when faced with option objects:
interfaceOptions { reverse? :boolean;
}
function whatever({ reverse = false }: Options = {}) {
console.log(reverse);
}
Copy the codeYou can say that the interface should warn or fail when an object passed to whatever has a property other than reverse. But in this case:
whatever({ reverse: true });
Copy the codeWe pass in an Object with attributes like toString, Constructor, valueOf, hasOwnProperty, etc., because this Object is an instance of Object. It might seem too strict to require these attributes to be “own” (this is not how objects work at runtime), but maybe we can make some exceptions for the attributes that come with Object.
Thanks to my podcast partner Surma for proofreading and suggestions, and Ryan Cavanaugh for correcting my TypeScript content.