Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ZoomConstantExpression to StylePropertyExpression #267

Closed
tangerine-orange opened this issue Jul 11, 2023 · 5 comments
Closed

Add ZoomConstantExpression to StylePropertyExpression #267

tangerine-orange opened this issue Jul 11, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@tangerine-orange
Copy link
Contributor

Describe the bug
In this piece of code from maplibre-gl-gs:

    _handleSpecialPaintPropertyUpdate(name: string) {
        if (name === 'line-gradient') {
            const expression: ZoomConstantExpression<'source'> = (this._transitionablePaint._values['line-gradient'].value.expression as any);
            this.stepInterpolant = expression._styleExpression.expression instanceof Step;
            this.gradientVersion = (this.gradientVersion + 1) % Number.MAX_SAFE_INTEGER;
        }
    }

we are coercing expression to ZoomConstantExpression<'source'>. This is required because expression is defined as a StylePropertyExpression = ConstantExpression | SourceExpression | CameraExpression | CompositeExpression.

This coercion tells me that there's some problem with the typing here. I see two options:

  1. expand StylePropertyExpression to include ZoomConstantExpression, and possibly ZoomDependentExpression as well. Currently, these are the only two expression types that StylePropertyExpression doesn't include.
  2. In mapbox-gl-gs codebase, set expression: StylePropertyExpression | ZoomConstantExpression<'source'>

I'd think option 1 is better, but I'm very familiar with the types here. @birkskyum do you have an opinion?


This issue came up when investigating maplibre/maplibre-gl-js#2837

@HarelM
Copy link
Collaborator

HarelM commented Jul 12, 2023

I think 1 is better, but we need to understand why it wasn't like that to begin with, i.e. is adding it to the type generalise it too much or is incorrect in some places...

@tangerine-orange
Copy link
Contributor Author

tangerine-orange commented Jul 12, 2023

agreed for sure. I will do some investigation, hoping that we will get some input from @birkskyum or someone else who knows.

@birkskyum
Copy link
Member

I don't know this area of the library well unfortunately.

@tangerine-orange
Copy link
Contributor Author

tangerine-orange commented Sep 12, 2023

Looking at this some more, I don't know if adding ZoomConstantExpression and ZoomDependentExpression to StylePropertyExpression is the right move. The structure of the zoom expressions is a bit different than the other style expressions. expressions are pretty complicated, with lots of nested expressions and different types of expressions.

Rather than just adding the zoom expressions to the StylePropertyExpression definition, a more comprehensive look at the typings of expressions is needed here.

For now, I think we should just expose an isZoomExpression type guard like @HarelM suggests in maplibre/maplibre-gl-js#2837.

@HarelM
Copy link
Collaborator

HarelM commented Sep 12, 2023

Fixed by #334

@HarelM HarelM closed this as completed Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants