fix!: improve types for {set,set}LayoutProperty, {set,set}PaintProperty to be the actual type instead of string/any#7481
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens TypeScript typings for runtime styling APIs so set/get{Layout,Paint}Property use style-spec-derived property keys and value types instead of string/any, aligning the public API with the MapLibre Style Specification.
Changes:
- Narrow
MapandStyleset/getPaintProperty+set/getLayoutPropertytokeyof AllPaintProperties/AllLayoutPropertieswith corresponding value types. - Update
StyleLayerproperty accessors/mutators to use the same typed keys/values and add typed entries for global-state paint ref tracking. - Adjust internal initialization paths to satisfy the stricter key/value typing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ui/map.ts | Updates Map styling APIs to be generic over AllPaintProperties/AllLayoutProperties keys and values. |
| src/style/style_layer.ts | Propagates typed paint/layout property keys through StyleLayer getters/setters and global-state ref tracking. |
| src/style/style.ts | Updates Style styling APIs to typed keys/values and threads those types through paint/layout update helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return this._transitionablePaint.getTransition(baseName) as AllPaintProperties[K]; | ||
| } else { | ||
| if (name === 'visibility' || this._unevaluatedLayout?.hasProperty(name)) { | ||
| if (this._unevaluatedLayout?.hasProperty(name)) { |
There was a problem hiding this comment.
now a ts error.. not sure if we want to keep these errors, this will error in TS code, but won't in js
|
|
||
| for (const property in layer.paint) { | ||
| this.setPaintProperty(property, layer.paint[property], {validate: false}); | ||
| this.setPaintProperty(property as keyof AllPaintProperties, layer.paint[property as keyof typeof layer.paint], {validate: false}); |
There was a problem hiding this comment.
all of these are nessary because for..in does not narrow the type for some reason
There was a problem hiding this comment.
Yeah, this sucks,not sure why typescript traspiler doesnt do that...
There was a problem hiding this comment.
there is a long issue. Marked as not planned.
For some reason, objects in JS don't have fixed keys or something and somehow keys may apear??
So string is the only valid key apparently??
CommanderStorm
force-pushed
the
tighter-types
branch
from
9f91080 to
ccbabb6
Compare
| return this._transitionablePaint.getTransition(baseName) as AllPaintProperties[K]; | ||
| } else { | ||
| if (name === 'visibility' || this._unevaluatedLayout?.hasProperty(name)) { | ||
| if (this._unevaluatedLayout?.hasProperty(name)) { |
There was a problem hiding this comment.
Are you sure about this condition change? I think it was there delibery, wasn't it?
There was a problem hiding this comment.
The other code now also triggers a TS error.
So not sure.. We can keep it if I as any, but that sounds hacky..
I added this because TS was "string".
|
I'm not really sure this is a breaking change to be honest, if your are passing incorrect values this should help you. |
technically not, but it will break lots of people who are using this wrong. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7481 +/- ##
==========================================
- Coverage 92.80% 92.80% -0.01%
==========================================
Files 290 290
Lines 24087 24055 -32
Branches 5099 5086 -13
==========================================
- Hits 22354 22324 -30
+ Misses 1733 1731 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Should remove a bunch of incorrect code looking at our codebase alone.
Definitively breaking given how many issues we had with this in our codebase too though.
tscslows down a bit because of this, but they are working on this part, so likely fine.