-
-
Notifications
You must be signed in to change notification settings - Fork 736
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
Improve the handling of map options #4145
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4145 +/- ##
==========================================
- Coverage 86.88% 86.87% -0.01%
==========================================
Files 242 242
Lines 33044 33075 +31
Branches 2011 2017 +6
==========================================
+ Hits 28709 28734 +25
- Misses 3374 3383 +9
+ Partials 961 958 -3 ☔ View full report in Codecov by Sentry. |
src/types/util.ts
Outdated
* See https://medium.com/terria/typescript-transforming-optional-properties-to-required-properties-that-may-be-undefined-7482cb4e1585 | ||
*/ | ||
|
||
export type Complete<T> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually thinking of removing this folder entirely, and move the types to either the utils folder or the files that are using those.
I also don't see this is used in another place, so I'm not sure there's an actual need to move it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer not to have too much noise in map.ts
but no strong opinion, I can do whatever you think is best.
One other unrelated question, do you know why const defaultMinZoom = -2;
?
I would love to add a comment there because it doesn't look intuitive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No clue why it's -2.
If you prefer no to put it in map.ts
I would recommend adding it to the utils file.
Also the tilejson and remove this folder completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-2 is to render maps < 512px (see mapbox/mapbox-gl-js#9028)
I don't have any objections to this PR in general. |
For now |
22aed2a
to
093c84c
Compare
It looks like some tests are failing... |
I'm not sure the failing test is your fault. I see it on a PR for a package update. |
src/ui/map.ts
Outdated
if (this.style) { | ||
this.style.setEventedParent(null); | ||
// transformStyle relies on having previous style serialized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bug fix? If so, I think it will be better to have this in a different PR...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only code factorization:
this.style
is tested on l 1732, 1737, and 1738 on the leftoptions.transformStyle
is tested l 1732 and 1737 on the left
src/ui/map.ts
Outdated
getImage(id: string): StyleImage { | ||
return this.style.getImage(id); | ||
getImage(id: string): StyleImage | undefined { | ||
return this.style?.getImage(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a behavior change? As far as I understand, before this change if style was not defined, this method would have thrown an exception, wouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline with @HarelM
Summary of the discussion: What is the expected behavior of this function:
- throw is not documented
- the type of
this.style: Style
does not let users think it might throw - is it valuable to throw here?
I think the behavior should be consistent across methods.
Some might throw:
getImage(id: string): StyleImage {
return this.style.getImage(id);
}
Some are noops when style
is undefined
redraw(): this {
if (this.style) {
// cancel the scheduled update
if (this._frameRequest) {
this._frameRequest.abort();
this._frameRequest = null;
}
this._render(0);
}
return this;
}
Some warn when style
is undefined
_updateDiff(style: StyleSpecification, options?: StyleSwapOptions & StyleOptions) {
try {
if (this.style.setState(style, options)) {
this._update(true);
}
} catch (e) {
warnOnce(
`Unable to perform style diff: ${e.message || e.error || e}. Rebuilding the style from scratch.`
);
this._updateStyle(style, options);
}
}
I believe this PR got too big... :-( |
src/ui/map.ts
Outdated
@@ -298,14 +301,15 @@ export type MapOptions = { | |||
* the schema described in the [MapLibre Style Specification](https://maplibre.org/maplibre-style-spec/), | |||
* or a URL to such JSON. | |||
*/ | |||
style: StyleSpecification | string; | |||
style?: StyleSpecification | string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that other improvements are great, I would avoid changing this as part of this PR, keep it as is for now and open a different PR just for this change (with its impact obviously).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the best way to move forward is to remove the last commit from this PR.
I can then submit another PR making MapOptions.style
optional and dropping all the other changes in that last commit as I don't think you find them useful?
src/ui/map.ts
Outdated
if (this.style) { | ||
this.style.setEventedParent(null); | ||
// transformStyle relies on having previous style serialized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only code factorization:
this.style
is tested on l 1732, 1737, and 1738 on the leftoptions.transformStyle
is tested l 1732 and 1737 on the left
src/ui/map.ts
Outdated
getImage(id: string): StyleImage { | ||
return this.style.getImage(id); | ||
getImage(id: string): StyleImage | undefined { | ||
return this.style?.getImage(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline with @HarelM
Summary of the discussion: What is the expected behavior of this function:
- throw is not documented
- the type of
this.style: Style
does not let users think it might throw - is it valuable to throw here?
I think the behavior should be consistent across methods.
Some might throw:
getImage(id: string): StyleImage {
return this.style.getImage(id);
}
Some are noops when style
is undefined
redraw(): this {
if (this.style) {
// cancel the scheduled update
if (this._frameRequest) {
this._frameRequest.abort();
this._frameRequest = null;
}
this._render(0);
}
return this;
}
Some warn when style
is undefined
_updateDiff(style: StyleSpecification, options?: StyleSwapOptions & StyleOptions) {
try {
if (this.style.setState(style, options)) {
this._update(true);
}
} catch (e) {
warnOnce(
`Unable to perform style diff: ${e.message || e.error || e}. Rebuilding the style from scratch.`
);
this._updateStyle(style, options);
}
}
Yes, let's drop the last commit and open an issue about the inconsistency of the methods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THANKS!
Thanks for your help Harel! |
Content of this PR:
type MapOptions
I'd like to get some feedback and I'll create an issue and fill out the launch checklist after that.
Thanks!
Launch Checklist
CHANGELOG.md
under the## main
section.