maplibre-gl-js icon indicating copy to clipboard operation
maplibre-gl-js copied to clipboard

Improve type safety across entire codebase

Open tomhicks opened this issue 4 months ago • 2 comments

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 anys 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.

tomhicks avatar Oct 01 '24 08:10 tomhicks