Fix setting line-gradient to null/undefined

[tangerine-orange]

This PR fixes #2683

Problem

A StyleLayer is added to a map by calling map.addLayer. In particular, we can add a LineStyleLayer that adds a color gradient (line-gradient) as follows:

    map.addLayer({
        type: 'line',
        source: 'line',
        id: 'line',
        paint: {
            'line-color': 'red',
            'line-width': 14,
            'line-gradient': [
                'interpolate',
                ['linear'],
                ['line-progress'],
                0,
                'blue',
                1,
                'red'
            ]
        }
    });

Our problem comes when we try to unset the line-gradient using Map.setPaintProperty:

map.setPaintProperty('line', 'line-gradient', null)

Note the third argument, null, which is meant to unset the property. (Note that null and undefined have same behavior in this case)
Our app crashes and we see an error in console:

Explanation

When we call map.setPaintProperty('line', 'line-gradient', null), our callstack is:

-->LineStyleLayer._handleSpecialPaintPropertyUpdate
->StyleLayer.setPaintProperty
Map.setPaintProperty

In LineStyleLayer._handleSpecialPaintPropertyUpdate, there is a special case for line-gradient.

        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;
        }

You can see that this code assumes the existence of expression._styleExpression. This assumption is broken when we're setting the property to null.

Solution

Check for the existence of _styleExpression before trying to access it.

Bonus

Map.setPaintProperty expects you to pass null when unsetting the existing value, not undefined. This is what we test for. I added this info to the documentation for this method.

Potential future work

1. Do we want to enforce the use of null over undefined for unsetting? I'd imagine we would do this using Typescript. If so I can make ticket.
2. Casting as ZoomConstantExpression doesn't seem ideal, especially since ZoomConstantExpression isn't even one of the possible types for this._transitionablePaint._values['line-gradient'].value.expression. We should probably clean up the typing here? If so I can make ticket.

Launch Checklist

• 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.
• Link to related issues.
• 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.
• Add an entry to CHANGELOG.md under the ## main section.

[tangerine-orange]

@HarelM This is my first time contributing to this project -- do I need to do something to start the CI jobs?

[HarelM]

I need to start it, I changed the settings now so that it won't happen in the future...

[HarelM]

Thanks for taking the time to solve this!
I think setting null and undefined should be possible and should behave the same, nice to have though.
If the typings aren't great I suggest to improve them as part of this PR.
If you prefer to have it under a different PR that completely fine too.

[tangerine-orange]

If you prefer to have it under a different PR that completely fine too.

I think I prefer to do it separate. I will make a ticket for it and link this PR. I suspect it might get pretty messy so I don't want to hold up this bugfix.

[tangerine-orange]

If you prefer to have it under a different PR that completely fine too.

I think I prefer to do it separate. I will make a ticket for it and link this PR. I suspect it might get pretty messy so I don't want to hold up this bugfix.

maplibre/maplibre-style-spec#267

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03% 🎉

Comparison is base (742d7a1) 75.03% compared to head (a322f55) 75.06%.
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2837      +/-   ##
==========================================
+ Coverage   75.03%   75.06%   +0.03%     
==========================================
  Files         240      240              
  Lines       19131    19134       +3     
  Branches     4318     4320       +2     
==========================================
+ Hits        14355    14363       +8     
+ Misses       4776     4771       -5     

Files Changed	Coverage Δ
src/ui/map.ts	83.23% <ø> (ø)
src/style/style_layer/line_style_layer.ts	57.89% <100.00%> (+11.59%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tangerine-orange]

@HarelM Are you okay with this branch as is or did you want me to do anything else before shipping?

[HarelM]

See my last comment. Otherwise approved.

[HarelM]

@tangerine-orange where are we with this PR?

[tangerine-orange]

@HarelM Sorry for the delay. I've added the type guard you suggest to maplibre/maplibre-style-spec#334. I integrated the updated maplibre-style-spec locally and it works as intended.

[tangerine-orange]

@HarelM okay, I integrated the new version of @maplibre/maplibre-gl-style-spec and am using the type guard.

[HarelM]

Am I missing a change in the package.json file? I see the luck file change, but not the package...?

[tangerine-orange]

Am I missing a change in the package.json file? I see the luck file change, but not the package...?

my bad, added.