Skip to content

global-state expression support in filter and paint properties #5613

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

Merged
merged 53 commits into from
May 20, 2025

Conversation

zbigniewmatysek-tomtom
Copy link
Contributor

@zbigniewmatysek-tomtom zbigniewmatysek-tomtom commented Mar 11, 2025

This PR adds a new map methods: setGlobalStateProperty() and getGlobalState() leveraging the new global-state expression.

Out of the scope: global-expression in layout properties (setGlobalStateProperty() will have no effect on those).

See the style spec update (needs to be merged first) and related issues:
maplibre/maplibre-style-spec#886
#4964

See example:

  • test/examples/filter-symbols-using-global-state.html

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.

@zbigniewmatysek-tomtom zbigniewmatysek-tomtom changed the title Global state expression [Draft] Global state expression Mar 12, 2025
@zbigniewmatysek-tomtom zbigniewmatysek-tomtom marked this pull request as ready for review May 8, 2025 11:15
@zbigniewmatysek-tomtom
Copy link
Contributor Author

@HarelM feel free to kill me afterwards - I missed one type in the spec: maplibre/maplibre-style-spec#1130 😅

Codecov was failing because fork main was out of sync with base, I will try to keep it in sync now.

@HarelM
Copy link
Collaborator

HarelM commented May 14, 2025

@HarelM feel free to kill me afterwards - I missed one type in the spec: maplibre/maplibre-style-spec#1130 😅

If I'll kill you, it will be hard to push this feature forward 😛

@HarelM
Copy link
Collaborator

HarelM commented May 14, 2025

Doesn't look like codecov approved the merge from main, not sure I know how to solve this... It probably requires some git magic...

@zbigniewmatysek-tomtom
Copy link
Contributor Author

I propose to solve codecov issue separately, I see that it's failing for most open PRs, also those with passed CI.
There are a lot of old issues reported about wrong head commit, missing head etc. Maybe it's worth trying CLI and https://github.com/codecov/codecov-cli#pr-base-picking

https://app.codecov.io/gh/maplibre/maplibre-gl-js/pulls?prStates%5B0%5D=OPEN
image

Apart from that, everything else is passing. Spec release is not strictly necessary, I've added @ts-ignore in one test, where type TS was complaining about missing global-state type.
After merging this PR, I need to create another PR to the spec to add ✅ in the global-state docs, so you could as well do the spec release afterwards fixing both things at the same time.

@HarelM
Copy link
Collaborator

HarelM commented May 15, 2025

Most PRs have code coverage, checkout the closed ones.
Can you maybe try opening a different PR, maybe from a different branch (with the same content) to see if we can get the code coverage report? It's important for me to make sure new code has tests covering it.
You might be able to run the coverage locally and see that the new lines are covered, but this might harder than trying a new branch...

If there will be a need to later on remove the @ts-ignore from the test, I prefer to release a new version of the style spec, maybe one that solves the globalstaterefs optional parameter as can be seen here: #5898

@HarelM
Copy link
Collaborator

HarelM commented May 15, 2025

@zbigniewmatysek-tomtom
Copy link
Contributor Author

I created new PR with squashed content of this one:
#5903

At least patch report was generated: https://app.codecov.io/gh/maplibre/maplibre-gl-js/commit/12ee82f5d0e988986406406fdab2ddc9d7f44fb2

@HarelM
Copy link
Collaborator

HarelM commented May 15, 2025

I've opened a bug and linked it to the above PR, let's see what codecov team will say.

@zbigniewmatysek-tomtom
Copy link
Contributor Author

zbigniewmatysek-tomtom commented May 19, 2025

@HarelM I run unit tests locally and compared:

main #5613
evaluation_parameters.ts 94.59% 94.87%
style.ts 85.77% 86.31%
properties.ts 82.53% 82.66%
map.ts 87.36% 87.44%
src/style_layer 98.28% 98.46%
src/data 67.32% 67.32%

@HarelM
Copy link
Collaborator

HarelM commented May 19, 2025

Thanks for looking into this!
Is there a way to make sure the newly added code is covered by tests somehow besides the statistics you just sent?

@zbigniewmatysek-tomtom
Copy link
Contributor Author

zbigniewmatysek-tomtom commented May 19, 2025

Most of the significant code changes are in few files.
Maybe you can run coverage locally and asses if the coverage is at the acceptable level?

image

@HarelM
Copy link
Collaborator

HarelM commented May 20, 2025

Can you resolve conflicts please? I'll merge this afterwards.

@HarelM
Copy link
Collaborator

HarelM commented May 20, 2025

Ok, one last question: are getLayoutAffectingGlobalStateRefs and _findGlobalStateAffectedSources well covered (>90%)?

@zbigniewmatysek-tomtom
Copy link
Contributor Author

@HarelM I ensured both methods have 100% coverage:

image
image

@HarelM HarelM merged commit 090c66a into maplibre:main May 20, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants