Fix filter typing and add expression type. #1383

cns-solutions-admin · 2022-07-20T13:57:24Z

See #1380.
Add typing for expressions and fix filter expressions.
improve filter conversion by adding typing and helping typescript to guess types.
add tests for filters mentioned in issue #1380.

no backports.

improve filter conversion by adding typing and helping typescript to guess types. add tests for filters mentioned in issue maplibre#1380.

github-actions · 2022-07-20T14:31:14Z

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

HarelM · 2022-07-20T15:33:20Z

Thanks for taking the time to do this!!
Any chance to add more tests? Seems like there are more code changes than tests changes, which I find odd.
Also, I'm not sure I understand the legacy part, can you elaborate on that?

HarelM · 2022-07-20T15:34:44Z

build/generate-style-spec.ts

+    | ['boolean', ...(unknown | ExpressionSpecification)[], unknown | ExpressionSpecification] // boolean
+    | CollatorExpressionSpecification
+    | ['format', ...(string | ['image', ExpressionSpecification] | ExpressionSpecification | {'font-scale'?: number | ExpressionSpecification, 'text-font'?: string[] | ExpressionSpecification, 'text-color': ColorSpecification | ExpressionSpecification})[]] // string
+    | ['image', unknown | ExpressionSpecification] // image


Why do we allow unknowns here?

According to the documentation, anything is allowed here, but it is not entirely clear - obviously not arrays, as they could not be distinguished from expressions.

So it's unknown for now, as we should not prevent anything that is allowed.

It can be improved later, but only after some expression tests have been created.

See my other comment about tests...
Since you are already deep into it I would hate to postpone it to a later time which might not come.
Let's fix it the best we can at this point in time i.e. add the relevant tests and improve the typings further. :-)

HarelM · 2022-07-20T15:35:21Z

src/style-spec/feature_filter/convert.ts

-        converted = convertHasOp(filter[1] as string);
-    } else if (op === '!has') {
-        converted = ['!', convertHasOp(filter[1] as string)];
+        converted = ['any', ...children];


Switch case?

I didn't change the overall logic - it was and is an if-cascade.
The diff view here rather shows it as deleted all and inserted a lot :-)

The changes here are only for cleaner typescript code that avoids type coercions as much as possible, e.g. for a ['any', ...LegacyFilterExpression[]] filter filter.slice(1) gives a ('any' | LegacyFilterExpression)[] (or worse), while [, conditions] = filter gives the correct LegacyFilterExpression[].

In the same way ['all', ...conditions] is a LegacyFilterExpression, while ['all'].concat(conditions) isn't.

BTW, these changes only concern the conversion of the old (pre-1.0, I think) filter syntax to the new syntax.

But why not change this to switch case? This particular place...

I don't understand - which particular place? It's a long if/else if/.../else and I didn't want to change any logic

The longest else-if you can :-)
Not changing logic can be achieved by adding the relevant tests, if those don't exists let's create them (I think it also addresses the other comment you wrote) :-)

cns-solutions-admin · 2022-07-21T06:08:49Z

As mentioned above, there are no real code changes, just replacing b = a.slice(1) by [, b] = a and [a].concat(b) by [a, ...b].
Additionally there were some weird type coercions "as string[]" where the elements definitely were not strings, etc.
This allowed typescript to do the type checking instead of using lots of type coercions.

The existing tests still pass.
Intersetingly I didn't find any tests testing the individual expression or filter functions!?

As to LegacyFilterExpression: that's the old (deprecated, legacy) syntax as described on https://maplibre.org/maplibre-gl-js-docs/style-spec/other/#other-filter. I decided to fully type it to make sure it was all either the old or the new syntax.

I would even think that that whole legacy filter part should be dropped from maplibre 2.
While upgrading from mapbox to maplibre 1.x was only a lot of renaming, upgrading to maplibre 2.x is rather hard (and currently not possible for us without some fixes like this) and therefore dropping a syntax that is deprecated for years, should be no problem. (#1385)

HarelM · 2022-07-21T08:15:45Z

2.x was already released with these legacy filters so the only proper way to discontinue them is in version 3, assuming we want to deprecate them...

cns-solutions-admin · 2022-07-21T09:03:49Z

They ARE deprecated since 1.0 or so (in the documentation). As to removing them, the correct version should probably be 3 (although 2.x does not feel quite stable to me yet).

cns-solutions-admin · 2022-07-22T06:05:53Z

Anything preventing the merge?

HarelM · 2022-07-22T06:57:10Z

I need an answer for the above code review questions :-)

cns-solutions-admin · 2022-07-22T07:22:46Z

Seems, my first comment was lost or I pressed the wrong button? there were other buttons than simply "Comment"!
I didn't see your other comment to my comment. And now I don't understand it.

cns-solutions-admin · 2022-07-22T08:08:52Z

I'm doing this in the context of a project for my company, where we are evaluating, if the 3D display might be usable and useful.
So far I was not even able to compile our project with maplibre 2.x (whereas migrating from mapbox to maplibre 1.5 was fine).

I can justify to my company improving maplibre regarding bugs (e.g. wrong typings) or introducing new features we would like or need, but I do not have time for

writing tests for those expressions which worked since mapbox 1.0 and the code of which did not change according to you
writing tests for a conversion of deprecated filters to above expressions, which we will never use (they are deprecated!)

Sorry about that, we try to improve OSS whereever possible, but in that case I will just add some type coercions in our code and try again, or if this does not work, we will have to postpone checking out maplibre 2.x.

HarelM · 2022-07-22T08:23:11Z

Thanks for the honest reply. I'll take it into consideration.

build/generate-style-spec.ts

src/style-spec/feature_filter/convert.ts

HarelM · 2022-07-24T07:26:20Z

I've merged this to a branch under the main repo.
I'll see if I can add some tests and fix the remaining comments.
Feel free to send more PRs against that branch.

Fixes #1380 * Add typing for expressions and fix filter expressions. (#1383) improve filter conversion by adding typing and helping typescript to guess types. add tests for filters mentioned in issue #1380. * Change else-if to switch case * lint * Update CHANGELOG.md * Added some tests, fixed failing test * Fix failing test * Fix lint * Fix lint * Fix test * Fix code and added tests * Remove links in changelog * Fix lint Co-authored-by: cns-solutions-admin <[email protected]>

Add typing for expressions and fix filter expressions.

70ad854

improve filter conversion by adding typing and helping typescript to guess types. add tests for filters mentioned in issue maplibre#1380.

cns-solutions-admin mentioned this pull request Jul 20, 2022

FilterSpecification type: missing "literal", etc. #1380

Closed

HarelM reviewed Jul 20, 2022

View reviewed changes

HarelM reviewed Jul 24, 2022

View reviewed changes

build/generate-style-spec.ts Show resolved Hide resolved

HarelM reviewed Jul 24, 2022

View reviewed changes

src/style-spec/feature_filter/convert.ts Show resolved Hide resolved

HarelM changed the base branch from main to 1380-filter-specification July 24, 2022 07:22

HarelM merged commit 45fee15 into maplibre:1380-filter-specification Jul 24, 2022

HarelM mentioned this pull request Jul 24, 2022

Improve filter specification typings #1390

Merged

8 tasks

HarelM mentioned this pull request Aug 14, 2022

Fix interpolate typings definition #1489

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix filter typing and add expression type. #1383

Fix filter typing and add expression type. #1383

cns-solutions-admin commented Jul 20, 2022

github-actions bot commented Jul 20, 2022

HarelM commented Jul 20, 2022

HarelM Jul 20, 2022

cns-solutions-admin Jul 22, 2022

HarelM Jul 22, 2022

HarelM Jul 20, 2022

cns-solutions-admin Jul 21, 2022 •

edited

Loading

HarelM Jul 22, 2022

cns-solutions-admin Jul 22, 2022

HarelM Jul 22, 2022

cns-solutions-admin commented Jul 21, 2022 •

edited

Loading

HarelM commented Jul 21, 2022

cns-solutions-admin commented Jul 21, 2022

cns-solutions-admin commented Jul 22, 2022

HarelM commented Jul 22, 2022

cns-solutions-admin commented Jul 22, 2022

cns-solutions-admin commented Jul 22, 2022

HarelM commented Jul 22, 2022

HarelM commented Jul 24, 2022

Fix filter typing and add expression type. #1383

Fix filter typing and add expression type. #1383

Conversation

cns-solutions-admin commented Jul 20, 2022

github-actions bot commented Jul 20, 2022

HarelM commented Jul 20, 2022

HarelM Jul 20, 2022

Choose a reason for hiding this comment

cns-solutions-admin Jul 22, 2022

Choose a reason for hiding this comment

HarelM Jul 22, 2022

Choose a reason for hiding this comment

HarelM Jul 20, 2022

Choose a reason for hiding this comment

cns-solutions-admin Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

HarelM Jul 22, 2022

Choose a reason for hiding this comment

cns-solutions-admin Jul 22, 2022

Choose a reason for hiding this comment

HarelM Jul 22, 2022

Choose a reason for hiding this comment

cns-solutions-admin commented Jul 21, 2022 • edited Loading

HarelM commented Jul 21, 2022

cns-solutions-admin commented Jul 21, 2022

cns-solutions-admin commented Jul 22, 2022

HarelM commented Jul 22, 2022

cns-solutions-admin commented Jul 22, 2022

cns-solutions-admin commented Jul 22, 2022

HarelM commented Jul 22, 2022

HarelM commented Jul 24, 2022

cns-solutions-admin Jul 21, 2022 •

edited

Loading

cns-solutions-admin commented Jul 21, 2022 •

edited

Loading