Display better message + emit error when bad args are passed to map camera events

[mollymerp]

Right now if you call map.zoomTo([0,0]) we get the 'failed to invert matrix' and the map breaks. We should emit a syntax error with a clear message.

Similarly, when you pass a coordinate pair to map.flyTo the map doesn't break, it just doesn't do anything. We should emit an error there as well and investigate other potentially affected methods.

cc @geografa

[ryanhamley]

Seems to happen on scroll zoom as well, as seen here #6486

Is this a bug that should be fixed or is it enough to surface better errors?

[HarelM]

I had the same issue when calling flyTo with zoom: null. since the stack trace doesn't reveal the origin of this error message I had to comment out pieces of my code until I found the root cause.
A good error message would've saved me a few hours.
I was migrating from openlayers where zoom is optional and thus was not aware this function can cause this error...

[acharyasourav]

As a beginner in opensource, I would like to work on this. Can you tell me where I should start looking?

[SnailBones]

Thank you @Sugarlust!

Just to make sure there's no confusion on this front, Mapbox GL JS is no longer fully open source. Here's the current license. You'll need to sign a contributor's agreement to submit a pull request.

The functions in question are in camera.js:

mapbox-gl-js/src/ui/camera.js

Line 319 in 2bc4599

zoomTo(zoom: number, options: ? AnimationOptions, eventData?: Object): this {

mapbox-gl-js/src/ui/camera.js

Line 1448 in 2bc4599

flyTo(options: EasingOptions & {preloadOnly?: boolean}, eventData?: Object): this {

You can look in map.js for examples of throwing error events:

mapbox-gl-js/src/ui/map.js

Lines 2195 to 2198 in 2bc4599

	this.fire(new ErrorEvent(new Error(
	`The width and height of the updated image (${width}, ${height})
	must be that same as the previous version of the image
	(${existingImage.data.width}, ${existingImage.data.height})`)));

Lastly, we need a unit test for this behavior, probably in map.test.js

The contributor guide will get you set up to develop the library, feel free to ping me with any questions not covered there and to tag me for review on your PR.

[acharyasourav]

Hey, @SnailBones I checked out the snippets of code you mentioned. Can you tell me what criteria I need to check to fire up the error message in those functions? Also what actually is OBJECT in

mapbox-gl-js/src/ui/camera.js

Line 1448 in 2bc4599

flyTo(options: EasingOptions & {preloadOnly?: boolean}, eventData?: Object): this {

[SnailBones]

Can you tell me what criteria I need to check to fire up the error message in those functions?

I think the criteria should be that the input to the functions is of the correct type. So we should throw the error if the first argument is not a number inzoomTo or object in flyTo.

Also what actually is OBJECT in

We use Flow for type-checking, it's similar to TypeScript. Objectis the Flow type of eventData. Type-checking in JavaScript validates the source code but doesn't prevent the user from passing in invalid inputs, which is why we need to throw an error in these cases.

[acharyasourav]

Thanks, @SnailBones, So all I need to do is add an if condition that checks:

• zoom is not a number in zoomTo

  mapbox-gl-js/src/ui/camera.js

  Line 319 in 2bc4599

  zoomTo(zoom: number, options: ? AnimationOptions, eventData?: Object): this {

• and in flyTo if eventData does not have an object

  mapbox-gl-js/src/ui/camera.js

  Line 1448 in 2bc4599

  flyTo(options: EasingOptions & {preloadOnly?: boolean}, eventData?: Object): this {

• and then simply add error messages for the following.

is that right?

Also if there's an IRC or discussion page please let me know.

[acharyasourav]

Hey, @SnailBones can you check these out. Do these sound correct what other changes should I make?

https://github.com/Sugarlust/mapbox-gl-js/blob/febd320d180aeaa0a3e2f20c3778dac7b8f5ad2c/src/ui/camera.js#L320-L329

https://github.com/Sugarlust/mapbox-gl-js/blob/febd320d180aeaa0a3e2f20c3778dac7b8f5ad2c/src/ui/camera.js#L1454-L1460

Also, where can I find the contributor's license to sign, and is there a discussion channel or IRC?

[SnailBones]

@Sugarlust looks like you're on the right track to me! I think the error messages could benefit from being more specific, e.g. "Map.flyTo accepts an object, not [type of input]"

The contributor's agreement will be available to you when you submit a PR.

[acharyasourav]

Hey, @SnailBones I made the error messages a bit more specific. Check these out

https://github.com/Sugarlust/mapbox-gl-js/blob/e4c22f4a8639d72f42b8d78dc85b142fce621048/src/ui/camera.js#L1454-L1460

https://github.com/Sugarlust/mapbox-gl-js/blob/e4c22f4a8639d72f42b8d78dc85b142fce621048/src/ui/camera.js#L320-L329

Also, should I make a PR now?

[SnailBones]

A PR would be great, thank you!

[acharyasourav]

@SnailBones I just made a PR #12098 can you please help me through it, I'm a bit confused?

[SnailBones]

Closing this ticket as fixing it in isolation goes against our usage of Flow for type checking.

From #12098 (comment):

While this is a useful exception on developer errors, we don't have precedent of type safety errors on other APIs. Type safety is done with static type annotations, and while I imagine that not all external developer adopt the same type checking as part of their tooling, it's not trivial to exhaust every possible type checks at runtime as this is better suited for static analysis. If we introduce this, we would start to be inconsistent with other APIs and probably be asked to adopt similar patterns elsewhere.

This issue is still tracked in #9485 and should be considered in the context of broader improvements to error handling.

Thank you for your time and contribution @Sugarlust!