Improve filter specification typings

[HarelM]

Launch Checklist

Improve typings for filter specifications, a PR in continue to #1383 that solves #1380.

• Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
• Briefly describe the changes in this PR.
• Include before/after visuals or gifs if this PR includes visual changes.
• Write tests for all new functionality.
• Document any changes to public APIs.
• Post benchmark scores.
• Manually test the debug page.
• Add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>.

[HarelM]

@cns-solutions-admin feel free to review my changes.
I actually found a lot of test that covers the changed code (the else-if -> switch case).
I'm not sure how to create tests for the changed typings and how to remove the unknown, I do feel like it's better than before...

[github-actions]

Bundle size report:

Size Change: 0 B
Total Size Before: 203 kB
Total Size After: 203 kB

Output file	Before	After	Change
maplibre-gl.js	193 kB	193 kB	0 B
maplibre-gl.css	9.65 kB	9.65 kB	0 B

ℹ️ View Details

No major changes

[cns-solutions-admin]

Ad testing the typings:

• write lots of expressions that should work and see if they compile
• (to test that our assumptions about what should work are correct, evaluate those expressions - the result is irrelevant, but it should not throw errors or return the default value)

Or just wait until somebody complains ;-)

[HarelM]

The question is more about removing the unknown mainly as I don't feel comfortable introducing it...
Having said that, I don't have a better solution, so this will probably stay this way until, as you said, someone complains.
I also feel like in most cases people write a json style file and don't write the styles in the code so these never actually get checked (for example I use ngx-maplibre which allows writing the styles in the html file and thus doesn't get typed checked).
So in general, this is a step forward...

[HarelM]

I tried to add the following test to the same file:

test('non-legacy exprssions', () => {
        function compileTimeCheck(filter: ExpressionFilterSpecification) {
            return isExpressionFilter(filter);
        }
        expect(compileTimeCheck(['any'])).toBeTruthy();
        expect(compileTimeCheck(['at', 2, ['array', 1, 2, 3]])).toBeTruthy();
        expect(compileTimeCheck(['at', 2, [1, 2, 3]])).toBeTruthy();
    });

But the test passes while the IDE (vs code) is showing the the last test it not "correct" from typescript point of view. Might be related to how jest and typescript are configured unfortunately :-(
FYI @birkskyum @wipfli

[cns-solutions-admin]

Ad expression test:

• ['at', 2, [1, 2, 3]] is not a valid expression, as it should be ['at', 2, ["literal", [1, 2, 3]]]
• but isExpressionFilter in reality tests, if it is not a legacy filter: "at" is not a valid legacy filter, thus it is an expression filter (albeit an invalid one)

The function isExpressionFilter is rather a isDefinitelyNotALegacyFilter and might theoretically return a wrong result:

• ['==', '1', '1'] is both a valid expression filter (although nonsensical) and a legacy filter (although a field name of '1' is highly improbable)

[HarelM]

I know that this is not a valid expression, but the tests do not say that in any way - i.e. they continue to run instead of giving a transpilation error or something similar.
From a short google search and from the research we did when converting the tests to jest I don't think it's possible to "test" for transpilation issues as they either exist or doesn't.
Bottom line, I don't know how to solve this out of the top of my head, if you have a good idea feel free to create one test that passes and one that fails and I can help out writing more tests...

[cns-solutions-admin]

As the types are compile-time only, you can only check the positive case, e.g. to test for valid legacy filters:

    test('valid legacy filters', () => {
        function compileTimeCheck(filter: LegacyFilterSpecification) {
            expect(true).toBe(true); // not really necessary
        }
        compileTimeCheck(['has', '$id']);
    });

Similar for expressions.

Anyway, there will probably always be expressions, that from a type perspective are correct, but due to the return types of feature properties are incorrect during evaluation.

Thus it is important that all valid expressions will pass the type check, but not necessarily that all expressions that pass the type check, are valid.

[cns-solutions-admin]

BTW, above "test" is not really a test, as it is successful, if it compiles.
It can only break by not compiling.

But it documents, what are valid expressions and will not compile, if somebody changes the type definition in an incorrect way.

[HarelM]

I would like to achieve the following which at this point I don't see how:

1. Add a new expression type in the typings, for example ['foo', number]
2. Write a test with this expression
3. Change the expression typings so that the above expression in no longer valid (i.e. remove it)
4. See that something fails this in a CI run (not in the IDE).

If you can suggest a way to accomplish this it will be great, otherwise adding these test won't protect us in the future in case we change the typings and break something...
I don't mind adding just "valid" expression tests, but I want to know that I'm protected in case I change the typings of the expression and break these tests...
As far as I tested, running npm run test-unit with a file that has transpilation issues doesn't fails the test or the file... :-(

[cns-solutions-admin]

I think this is exactly, what you achieve with these "tests". They just do not fail by failing, but by not compiling.

Ah, didn't see, that the transpilation does not fail. Hm.

[cns-solutions-admin]

The problem is that jest uses @swc/jest instead of babel-jest or ts-jest for transforming the typescript code.
Babel-jest does not work, "value as ..." is invalid.
But I was successful with ts-jest:

With package.json changes:

    "jest": "^28.0.3",
    "jest-environment-jsdom": "28.1.3",
    "ts-jest": "^28.0.7",

and jest.config.js changes:

    'transform': {
        '^.+(feature_filter)\\.test\\.ts$': 'ts-jest',
        '^.+\\.[tj]sx?$': '@swc/jest',
    },

it throws compile errors, if a type test does not compile.

As a lot of tests do not compile, only the relevant ones should be transformed by ts-jest (here feature_filter.test.ts).
If the tests are cleaned up to be valid typescript, all files can be compiled, but this will increase test time a lot.

Tested with the following test in feature_filter.test.ts:

    test('valid legacy filters', () => {
        function compileTimeCheck(filter: LegacyFilterSpecification) {
            expect(true).toBe(true);
        }
        compileTimeCheck(['has', '$id']);
    });

Replacing 'has' with 'hax' fails the test (compile error).

[HarelM]

The fact that a lot of tests do not compile isn't good... I'll see if I can spend some time fixing them.
Another alternative is adding a CI step that simply runs tsc on the code base in order to make sure transpilation is passing and thus avoid the penalty of running the tests slower with ts-jest (assuming this is actually faster)...

[HarelM]

I think I found the issue talking about swc not being a type checker: swc-project/swc#571
This might explain this issue.
I'll open a separate issue about it.

[cns-solutions-admin]

From a testing perspective in most cases it does not really matter, if the test code is correct typescript code as long as it tests what it should test.

However, for those tests that test typings (and those are rather few), we have to make sure that they are correct typescript code and are also type checked. This can be selectively done as described above and only marginally effects unit test speed.

[HarelM]

I'll wait for the following PR to be merged in order to add the relevant tests and later on merge this: #1399

[HarelM]

I've merged the main branch after the migration to ts-node and it seems the either the test code is not accurate or there are some problems with the definition of the types that were introduced.
@cns-solutions-admin any chance you can take a look at this?
The main issue I see is with the light.test.ts that the test is now "not compiling", but if I change it to anything that passes compilation the test fails at runtime, so I think there's a problem with the typings...
Feel free to prove me wrong...

[cns-solutions-admin]

Whoever wrote this test, tested that "Sh* in, sh* out", i.e. if something is passed in that cannot be understood, it passes through unchanged.

According to the documentation valid colors are strings (see https://docs.mapbox.com/mapbox-gl-js/style-spec/types/#color), e.g. '#888' (= '#888888'), '#808080', 'red'.
The result, when the value can be interpreted as color, seems to be an object {r, g, b, a}.
However, the calculation seems to have rounding problems, as '#808080' is returned as r = g = b = 0.5019607843137255 instead of r = g = b = 0.5.

So a valid test would be

        light.setLight({color: 'red'}, {validate: false});
        expect(light.properties.get('color')).toEqual({a: 1, r: 1, g: 0, b: 0});

So, the current test is bad, the typing is correct. And you can't write such a bad test in future!

P.S. unit test before (typescript checks): 34s, after: 238s, but second time just 70s, so not too bad.

[HarelM]

Yea, I saw this issue too, I'm wondering if it's worth just type checking instead...

[HarelM]

Any change you could fix the tests so I could merge this?

[HarelM]

I've added a few tests, which I checked are now failing if there's a transpiration error.
I've fixed the test that was failing.
If you could write here some more valid expressions or send a PR to the relevant branch it would be great as I'm no sure I know how to cover some of the cases that you had which were problematic...

[birkskyum]

Thank you for adding so many types to this. LGTM

[HarelM]

@cns-solutions-admin any chance for some help here in giving more use cases we can test before this is merged?

[cns-solutions-admin]

I think you could add a "test" (i.e. if it compiles) for each of the expressions, but I don't think it's worth the effort.
If some expression that should work, does not work, you can still pass it as "as any as ExpressionSpecification".
And as far as I see there are no real tests for the calculation of expressions themselves, too.

[HarelM]

I added a few tests that checks for compilation correctness as part of this PR, I think it worth the effort. Maybe not cover all cases, but at least cover some, or some complicated once at least.
You are probably correct assuming there's no full coverage for all expression types and evaluation, there are some though.
But It would still help to add more compilation tests to cover more cases and also have this as "documentation in tests" kind of thing...
You don't have to write the test, just past here a few example so I can add to the tests.
The examples you used to build the typings, assuming this is what you did...

[cns-solutions-admin]

Examples from our configurations (property names added for context only):

"line-color": ["case", ["has", "color"], ["get", "color"], "white"]

"text-field": ["case", ["all", ["has", "point_count"], ["<", ["get", "point_count"], 3]], ["get", "cluster_routes"], ""]

"circle-radius": ["case", ["has", "point_count"], ["interpolate", ["linear"], ["get", "point_count"], 2, 18.0, 10, 24.0], 12.0]

"circle-color": [
    "case",
    ["has", "point_count"], ["interpolate", ["linear"], ["get", "point_count"], 2, "#ccc", 10, "#444"],
    ["has", "priorityValue"], ["interpolate", ["linear"], ["get", "priorityValue"], 0, "#ff9", 1, "#f66"],
    "#fcaf3e"
]

"icon-image": [
    "case",
    ["==", ["get", "CAPITAL"], 1], "city-capital",
    [">=", ["get", "POPULATION"], 1000000], "city-1M",
    [">=", ["get", "POPULATION"], 500000], "city-500k",
    [">=", ["get", "POPULATION"], 100000], "city-100k",
    "city"
]

Filter:

"filter": ["match", ["get", "TYPE"], ["TARGETPOINT:HOSPITAL"], true, false]

"filter": ["match", ["get", "TYPE"], ["ADIZ", "AMA", "AWY", "CLASS", "NO-FIR", "OCA", "OTA", "P", "RAS", "RCA", "UTA", "UTA-P"], true, false]

"filter": ["==", ["get", "MILITARYAIRPORT"], 1]

[HarelM]

Both interpolation does not pass "compilation" from what I understand. I'm not sure if the types are incorrect or the expression... :-(

[cns-solutions-admin]

There is a missing ] for 'within' and an additional ] for interpolate.
Correct is:

    | ['within', unknown | ExpressionSpecification]
    // Ramps, scales, curves
    | ['interpolate', InterpolationSpecification, 
        number | ExpressionSpecification, number | ExpressionSpecification, ExpressionInputType | ExpressionSpecification, 
        ...(number | ExpressionInputType | ExpressionSpecification)[]]

[HarelM]

And that's why you need tests :-)