Improve typescript typings with strict null check

[HarelM]

This is an initial commit.
Some things to take into consideration:

1. In most cases it's enough to add a ! in order to solve the undefined part.
2. When deleting a property the property should be defined with ?
3. Add typings to functions as much as possible, your future self will thank you.
4. When importing, use import type so that we only import the relevant needed type.

Generally speaking, there shouldn't be any logic changes.

You can use npm run typecheck to see a summary of the remaining issues.

I think PRs against this branch would be the right way to move forward in terms of review

Resolves #2588

[drwestco]

Using the ! operator partially defeats the purpose of strictNullChecks, and should only be done as last resort. Here, I think it would make sense to use union types for Property, so the different property types are declared with the appropriate fields:

type BooleanProperty = {
    type: 'boolean';
    name: string;
}; 

type EnumProperty = {
    type: 'enum';
    name: string;
    values: string[];
    ...
};

type Property = BooleanProperty | EnumProperty | ... ;

[HarelM]

Feel free to pitch in 😀 I'll be happy to review any PR sent against this branch.

[HarelM]

The more I play around this the less value I see in pushing this forward honestly.
I'm mainly adding ! in places where the code is expected not to have null or undefined but the transpiler is saying that it can be undefined.
Littering the code with !, | null, | undefined is not a great outcome.
While this might uncover some bugs or badly defined types in most cases is only adds noise.
I'll see if there's a good way to make sure that the generated maplibre-gl.d.ts file is using strict mode and add this to the CI instead of waisting a lot of energy in something that won't really help.
If anyone else has other opinions around this I'll be happy to hear.

[neodescis]

I agree with your assessment. I think that strictNullCheck is one of the least useful of the strict checks, especially in terms of introducing it to an existing project. We may want to look into the other strict checks though.

[drwestco]

It is difficult to adopt in a mature project, with years of loose type definitions to account for. Littering the code with ! assertions is the wrong way to do it, though. Whether it's worth the effort to improve the actual type definitions is a judgment call. When turned on, strictNullChecks will correctly flag potential bugs.

        for (const feature of features) {
            const featureKey = feature.key;
            // Use of featureKey without null check flagged by compiler
            if (seenFeatures[featureKey.bucketInstanceId] === undefined) {
                seenFeatures[featureKey.bucketInstanceId] = {};
            }
            if (seenFeatures[featureKey.bucketInstanceId][featureKey.featureIndex]) {
                continue;
            }

If feature.key is guaranteed to be non-null, the type shouldn't indicate that it's optional. If it is optional, this code should check. The issue here is that a common QueryResult type is used for both grid.query and grid.hitTest, and key is only possibly null for the latter. That's sloppy. grid.hitTest should return a different type.

[HarelM]

Don't get me wrong, I'm all for improving the typings, but I find that the errors are not accurate in some cases, especially when the code is a bit more complicated like the following:

maplibre-gl-js/src/data/bucket/line_bucket.ts

Line 300 in b45b5b4

let prevVertex: Point;

Adding some additional types for specific cases also isn't a great solution, defining TypeA and then TypeAButWithoutB just to satisfy this check doesn't feel in some cases like the right solution.
There are places, as you said which makes sense, but not all of them.

What I can think of as a middle ground is to enable this check, fix the places where types are missing, incorrect, or needs attention and improve the code just a bit as separate PRs and then decide what to do on the remaining places and whether or not to introduce this flag.

Having said that, making sure the public API is strict can be a lot easier, as I did in #2623.

[HarelM]

I'm not planning to follow this through.
If anyone else is interested, feel free.