I recently upgraded all the packages in our very large monorepo to the Ember 6.1 app blueprint. The pretty modest file-by-file diff that appears to just upgrade some dependencies and switch to the new eslint flat config file format belies some very exciting and impactful changes to the code validations that eslint performs.
My pull request that upgrades to Ember 6.1 and updates all of our code to comply with the new default linting configuration included 13 commits containing changes to 12,100 lines of code across 1,155 files! This was virtually all driven by the change from @typescript-eslint's recommended configuration to its recommendedTypechecked configuration.
I learned a lot in the process of updating all of our code to comply with the new linting configuration, and I wrote a fairly long and detailed PR description to share what I learned with my team, so I figured I'd share it with the broader community in case it's helpful for others. And here it is!
Ember 6.1 includes some big linting updates, described below. So this PR updates us to Ember 6.1, including some miscellaneous/minor blueprint updates, but the bulk of it is making our codebase conform to the new linting rules.
New linting setup
One very notable change in the linting setup is that for ts and gts files, we've started using linting with type information, which allows us to use linting rules that are much more powerful and helpful. The major implications of this are described below.
@typescript-eslint/no-unused-vars
This rule was updated to also flag unused variables in catch statements, so where we used to do
try {
doSomething();
} catch (e) {
// ignore
}
we can now omit the variable declaration entirely and use a syntax I didn't even realize was allowed:
try {
doSomething();
} catch {
// ignore
}
@typescript-eslint/unbound-method
This rule helps us make sure we don't call methods with this bound incorrectly, e.g.
class Thingy {
private readonly obj = { key: 'value' };
setObjValue(val: string) {
this.obj.key = val;
}
}
let thingy = new Thingy();
let setObjValue = thingy.setObjValue;
// whoops, `this` will be `null`, so `this.obj.key`
// with throw an error
setObjValue('hello');
When I was fixing up our code to comply with this rule, I encountered some "false positives" that I addressed in a few different ways, and are informative for writing code in the future to comply with this rule.
The false positives mainly have to do with the fuzzy distinction between a class method and a property that happens to be a function. Consider:
class Thingy {
classMethod() {}
readonly propertyThatIsAFunction = () => {};
}
const thingy = new Thingy();
// unsafe because if `classMethod` references `this`, calling
// `classMethod()` won't have `this` bound correctly
const classMethod = thingy.classMethod;
// safe because arrow functions have `this` "pre-bound"
const propertyThatIsAFunction = thingy.propertyThatIsAFunction;
This doesn't just apply to classes, but also to objects, i.e. the above would be exactly the same with an object in place of the class & class instantiation:
const thingy = {
classMethod() {},
propertyThatIsAFunction: someFunction,
};
So, when this this rule flags something you are doing as an error, you can create an arrow function:
const classMethod = () => thingy.classMethod();
or bind it when saving it off:
const classMethod = thingy.classMethod.bind(thingy);
or if you have access to the original class and it seems to make sense, convert the method itself in the class definition to an inline arrow function:
class Thingy {
readonly classMethod = () => {};
}
// or
const thingy = {
classMethod: () => {};
}
any-related linting
One of the risks of using values typed as any is that they can "leak" out into a much bigger footprint of code than it seems like. For example,
// Suppose `create-object` is typed so that `createObject()` returns
// `{ value: string }`
import createObject from 'create-object';
let object = createObject();
// fine
object.value;
// type error
object.subObject.value;
Now, if create-object lacked type information,
// @ts-expect-error no types
// createObject is typed as `any`
import createObject from 'create-object';
// `object` is typed as `any`
let object = createObject();
// fine
object.value;
// also fine at compile-time, but will throw an error at runtime
object.subObject.value;
Basically, as soon as you have a value typed as any, any values you derive from it (via property accesses, function calls, etc etc) will be any and the compiler won't be able to do any meaningful type checking on those values.
We've had @typescript-eslint/no-explicit-any enabled for quite some time, but this just prevents us from explicitly typing a value as any. There are a bunch of cases where a value can be implicitly typed as any, and this rule is meant to "contain the damage."
There a many many cases where this rule flags errors, and many ways of addressing those errors, but here are some common ones I ran into:
inherently untyped values
When calling JSON.parse() or await fetchResponse.json() or the like, there is no static type information -- we, the programmer, have to tell the compiler what kind of value we expect to get from deserializing the JSON string into an object. So here, we just have to cast it, e.g. let myResult = JSON.parse(str) as MyResult;. Sometimes the type isn't defined as a named type and it doesn't seem worthwhile to create a named type, so you can just do let { value } = JSON.parse(str) as { value: string };.
catch blocks
In JavaScript, you can throw anything, not just Error objects. throw 'whoops' or even throw undefined are perfectly valid (although bad practice, and we have a linting rule to prevent us from doing it in our code). So in catch blocks, even though we generally treat the thrown value as an Error, we don't actually know what it is.
Up until recently in our TypeScript configuration (where we had useUnknownInCatchVariables disabled), variables in catch blocks were typed as any (mainly to ease the transition of code from un-typed JavaScript where we play fast and loose with our types to TypeScript). So this new rule flags pretty much any (hah!) usage of the caught variable as an error, and requires an explicit cast. NB, once this was done it wasn't much additional work to enable useUnknownInCatchVariables, which I did, so now these variables in catch blocks are typed as unknown and the compiler (as opposed to the linter) won't let us do anything with them without casting/type-narrowing.
To be as risk-averse as possible, we should ideally always use instanceof checks or type guards or whatever to ensure that the thrown thing is of a type that is compatible with what we expect, e.g.
try {
doThing();
} catch (e) {
if (e instanceof Error) {
reportErrorMessage(e.message);
} else if (typeof e === 'string') {
reportErrorMessage(e);
} else {
reportErrorMessage('unknown error');
}
}
But this is a big pain, and given that we lint against throwing non-Error objects for our code, it should generally be an edge case, so in cases like the above I pretty much always did something like
try {
doThing();
} catch (e) {
reportErrorMessage((e as Error).message);
}
and I think that's fine for us to do in general unless we're in an area of the code where we're particularly worried about what might be thrown (e.g. from external code) or want to be extra defensive.
error-typed and poorly-typed values
By "error-typed values" I mean ones where we use @ts-expect-error to tell the compiler to not worry about the fact that we're doing something it doesn't understand, and then it types any values that come out of that statement as any. By poorly-typed values I mean places where we intentionally use "any" such as the context in the ModalManager (typically because we haven't yet taken the effort to figure out how to type it correctly).
In both of these cases, if you can't fix the underlying problem, just cast. For example,
// @ts-expect-error no types
import createObject from 'create-object';
let obj = createObject() as {
key: string;
stats: { count: number }
};
let count = obj.stats.count;
or even (although the former is probably preferable in most cases)
// @ts-expect-error no types
import createObject from 'create-object';
// leave it typed as `any`
let obj = createObject();
// we don't care about `key` here, so don't bother to include it
let count = (obj as { stats: { count: number } }).stats.count;
promise/async-related linting
We now have linting rules that verify a bunch of things related the async/await and promises, and force us to be more thoughtful and intentional about our async/promise handling, which produces safer code. The two most significant rules are:
@typescript-eslint/require-await
This rule requires that async functions either have an await statement in them, or explicitly return a promise. When this rule flags an error, you probably want to just remove the async keyword from the function, making it synchronous. However, in some cases you might be passing the function to an API that requires a promise-returning function, so just removing the async keyword produces a type error. In this case you can leave the async and wrap any return values in Promise.resolve() e.g. return Promise.resolve('result') or just return Promise.resolve() if the function returns void.
@typescript-eslint/no-floating-promises
This rule prevents us from "ignoring" promises. For example, if you call an async function, you have to do one of the following:
// await the returned promise
await asyncThing();
// store the promise in a variable
const promise = asyncThing();
// pass the promise to a function
handlePromise(asyncThing());
// apply a `.catch()` handler to it
asyncThing().catch((e) => /* ... */);
// explicitly ignore it with the `void` operator
void asyncThing()
This is meant to ensure that we don't accidentally discard promises that we should be handling, both to prevent async/race condition/timing errors, and to make sure that if the promise rejects, it isn't an unhandled rejection (which can cause us to miss errors or, in Node, to cause the process to exit).
This rule forces us to think more carefully about our use of asynchronous functions, and whether they are actually asynchronous at the API level, or whether they just have asynchronous behavior as an internal implementation detail. Consider:
async function getFromServer(url: string) {
let response = await fetch(url);
return response.json();
}
async function setAndTrack(obj: Thing) {
obj.value = 'hello';
// get some stats from the object (async operation) and send them to a metrics service
let stats = await obj.getStats();
await sendTracksToMetricsService(stats);
}
In the first case, the caller would await the function call, and the async-ness is actually a part of the function's API -- getting from the server is inherently asynchronous and the caller needs to wait for the result. In the second case, the caller would not await the function call because metrics tracking is a side-effect and the fact that it's asynchronous is an implementation detail. So the second function's API is synchronous, we are just making the function async so we get the convenient await syntax, rather than having to use promise chaining:
function setBroadcast(obj: Thing) {
obj.value = 'hello';
// get some stats from the object (async operation) and send them to a metrics service
obj.getStats().then((stats) =>
sendTracksToMetricsService(stats)
).catch((e) => /* handle error */);
}
So, if we had the original async implementation of setBroadcast, it would be correct to call it without awaiting it because the caller doesn't want to wait for the metrics tracking to complete before proceeding. But this would trigger a linting error. In this case, it's telling us that the function's API should really be synchronous, and we should rewrite the function to not be async (rather than having all the call sites add a void or catch()).
In this case, we can have our cake and eat it too using an async IIFE:
function setBroadcast(obj: Thing) {
obj.value = 'hello';
(async () => {
// get some stats from the object (async operation) and send them to a metrics service
let stats = await obj.getStats();
await sendTracksToMetricsService(stats);
})().catch((e) => /* handle error */);
}
In cases where we truly do want to invoke an async function but not wait for it to complete before proceeding, it's probably best to
doAsyncThing().catch(captureException);
or in very rare cases where we're pretty darn confident it's not going to throw an error
void doReallySafeAsyncThing();
Top comments (0)