Add Feature Properties Transform

[wipfli]

Proof-of-concept implementation for #4198. The implantation is inspired the addProtocol implementation.

Demo:

https://github.com/wipfli/maplibre-feature-properties-transform-example

Launch Checklist

• [x] 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.

[wipfli]

This is ready for review. Let me know what needs to be changed

[HarelM]

Thanks again for the PR! Can you please resolve conflicts and also create some tests and an example page of how to use this feature?

[wipfli]

For the tests I will need some help. Do we already have tests for maplibregl.importScriptInWorker()?

[HarelM]

Import script requires integration tests, so I wouldn't start with that. Try unit testing the worker and the tile parser by mocking and spying a lot if the stuff. Also, don't forget the changelog entry.

[HarelM]

I've opened the following PR: https://github.com/wipfli/maplibre-gl-js/pull/341 Hope it helps...

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.79%. Comparing base (b8cb683) to head (2b2d199). Report is 578 commits behind head on main.

Files with missing lines	Patch %	Lines
src/source/worker.ts	40.00%	2 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4199      +/-   ##
==========================================
- Coverage   87.85%   87.79%   -0.07%     
==========================================
  Files         246      246              
  Lines       33372    33392      +20     
  Branches     2182     2183       +1     
==========================================
- Hits        29318    29315       -3     
- Misses       3064     3084      +20     
- Partials      990      993       +3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[HarelM]

The image is empty, I think it's a bug in chrome or headless chrome... not sure. I think the following arguments may help: https://github.com/maplibre/maplibre-gl-js/pull/4299/files

Or you can toggle headless for image creation.

[HarelM]

Now you have a failing test that reproduces what I said :-)

[HarelM]

The example is a way to inline everything into a single file for demo purposes, I would not advise to use createObjectURL in production, but instead use a javascript file stored next to the code or even on a CDN. This file can be created using the regular typescript tooling to support type check and everithing. I wouldn't like to complicate the import script interface for this edge case. This is also outside the scope of this PR I believe.

[louwers]

Ah OK, I thought that was the recommended approach. Makes sense. 👍

[wipfli]

The documentation for queryRenderedFeatures says that the result "contains the properties of its source feature":

https://github.com/maplibre/maplibre-gl-js/blob/a9420817af8507a9884bc4bfbb4318cc70f3761e/src/ui/map.ts#L1519

Based on this I would suggest to add a note to the documentation of the setFeaturePropertiesTransform function that changes will not be reflected in queryRenderedFeatures. For tests I would suggest to write some render tests. What do you think?

[HarelM]

I think that you'll have a hard time creating render tests for this feature as render tests are mainly style driven and this feature "breaks" the style. Browser integration tests are enough.

Can you elaborate on the limitation of query render features? If this hook is creating a discrepancy between the rendered results and the returned value of the query it might confusing to users, wouldn't it?

[wipfli]

The functionality proposed in here seems to have a lot of overlap with feature-state. It is just global on all features and not on individual features. I will think a bit if there is a better api than what is proposed here...

[HarelM]

@wipfli is this still something you want to push forward? Are any breaking changes needed while we are at version 5 pre-release for this feature?

[wipfli]

Thanks for the ping @HarelM. I think this would be a nice feature but I have not been thinking about it for a while. The main open question was the result from queryRenderedFeatures right? Regarding v5, this pull request should not lead to breaking changes so it can come after

[HarelM]

Yes, I believe the issue was around queryRenderedFeatures. Thanks for the update.

[neodescis]

Closing in favor of https://github.com/maplibre/maplibre-gl-js/pull/5370