maplibre-gl-js
maplibre-gl-js copied to clipboard
Improve type safety across entire codebase
User Story
As a developer working on the MapLibre source, I want to be able to make changes with the full support of static type analysis, in order to move faster breaking less things, and being able to safely remove cruft as I go.
As a developer consuming MapLibre, I want the types exposed to me to be correct, minimal and truthful, so that I can consume MapLibre more confidently.
Rationale
While working on a fix, I turned on strict: true
, noImplicitAny: true
and an eslint rules that helps eliminate cruft (https://typescript-eslint.io/rules/no-unnecessary-condition/) and I discovered that there are thousands of places that violate these rules.
This gives rise to the following problems in the codebase:
1. Unnecessarily defending against values that can never be null
Example:
function doSomething(param: {a: number}) {
if (param) {
doOneThing();
} else {
doAnotherThing();
}
}
In this example, either param
should be typed as {a: number} | undefined
if we must guard against 3rd-party consumers calling with the parameter missing, or the condition should be removed.
This kind of thing leads to unnecessary complexity in the code, requiring thinking about code paths that can never be executed. Or, it leads to dangerous refactors where we are actually expecting a user to call this with undefined.
This is often due to any
s sneaking into the code, such that there are code paths that pass undefined
here but the compiler isn't aware of them, so we end up defending against these phantom cases. Even if the call site is cleaned up to ensure the parameter is always passed, these conditions get left over, unexecuted forever.
2. Propagating any
types
function getSomething(params: Array<string>) {
const result = [];
params.forEach((param) => {
result.push(param.toUppercase()); // you can push anything here, it's very easy to make mistakes
})
return result; // we're expecting this to be Array<string> but it's actually Array<any>
}
// this requires a specific shape to be passed into it
function useSomething(params: Array<{a: string}>) {
}
// this is ultimately passing Array<string> where we should be passing Array<{a: string}>
// but this mistake is completely invisible to the compiler.
// Even worse, the result of this is now `any` so every call to this function loses type safety.
function doAThing() {
const somethings = getSomething(whateverParams);
return useSomething(somethings); // we've completely lost type safety by this point because somethings is Array<any>
}
In this example, in a couple of calls, we've completely lost type safety for any caller to a function. This makes it very easy for mistakes to sneak into refactors.
3. Incorrect checks
if (!layerIDs.filter(id => filterLayerIDs.has(id))) {
// this will always be executed
}
This actually happened in #4777 where an eagle-eyed reviewer spotted the mistake. Array.prototype.filter
always returns an array which is always truthy. It was supposed to be checking either the length of the array or using Array.prototype.some
instead.
It would be nice to have the compiler check these things for us, instead of relying on manual code inspection.
Impact
Bugs end up in production code.