Fix MapOptions#style is optional in the Map constructor #4003
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4003 +/- ##
==========================================
- Coverage 86.64% 86.44% -0.21%
==========================================
Files 242 242
Lines 32493 32502 +9
Branches 1990 2118 +128
==========================================
- Hits 28154 28096 -58
- Misses 3391 3482 +91
+ Partials 948 924 -24 ☔ View full report in Codecov by Sentry. |
| @@ -387,7 +387,8 @@ const defaultOptions = { | |||
| crossSourceCollisions: true, | |||
| validateStyle: true, | |||
| /**Because GL MAX_TEXTURE_SIZE is usually at least 4096px. */ | |||
| maxCanvasSize: [4096, 4096] | |||
| maxCanvasSize: [4096, 4096], | |||
| style: '', | |||
There was a problem hiding this comment.
If style can be undefined why setting it to an empty sting?
There was a problem hiding this comment.
If style can be undefined
This is an ambiguous statement. options.style can not be set to undefined but the style key can be omitted from options.
Then the reason is
Lines 332 to 333 in d76c044
i.e. the value after the default are applied should satisfy StyleSpecification | string
There was a problem hiding this comment.
CompleteMapOptions#style is a required key of type string | StyleSpecification.
If the style prop is not set in the options passed to the ctor, it has to be added with a tour satisfying string | StyleSpecification.
Does this make sense?
There was a problem hiding this comment.
Why are you adding style: '' though if you're making it optional above?
There was a problem hiding this comment.
The current behavior is that the default style value is undefined, setting it to '' will change the current behavior and has a higher probability of breaking exiting client code than keeping it as before.
Am I missing something?
There was a problem hiding this comment.
setting it to '' will change the current behavior and has a higher probability of breaking exiting client code than keeping it as before.
It all depends what you consider the source of truth. I am definitely a TS developer and consider types the source of truth and I like to use TS strict mode. I you think the average user look at the source code rather than than the TS types, you might be right.
There was a problem hiding this comment.
It's not that I think developers look at the code, but if you have something that is working right now, this might break it, changing only the types will make it more correct and might cause a break in the build, but not in run time.
It's a slim chance, but if the purpose of this change is to improve the public facing API types, let's focus on changing the types only at this point and avoid unexpected changes to the "logic".
Not sure I'm successful in passing my point here...
There was a problem hiding this comment.
Can we discuss on slack (victor at suumit.com). I'm busy tonight but should be available tomorrow. Thx.
There was a problem hiding this comment.
Are you in the maplibre slack? I can't find you using a simple "victor" search... 😕
See `if (options.style) this.setStyle(...);` in the constructor
vicb
force-pushed
the
MapOptions/style
branch
2 times, most recently
from
3ce06f0
to
640a762
Compare
|
I believe this was addressed by #4151, right?
|
|
You're absolutely right, sorry I forgot to close this one. |
See
if (options.style) this.setStyle(...);in the constructorMy use case is to use a base layer switcher so I do not want to set a style explicitly when instantiating the map.
Launch Checklist
CHANGELOG.mdunder the## mainsection.