set zoom guard, fixing illegal matrix errors

[hyperknot]

Launch Checklist

• briefly describe the changes in this PR
• write tests for all new functionality
• document any changes to public APIs
• post benchmark scores
• manually test the debug page
• tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

No changes, no new functionality, just a guard to make sure that zoom cannot be set to NaN, and thus turn the map into a frozen state.

Detailed description about how can set zoom receive a NaN value:
#6782

Those issues can be fixed in the future, if needed, but this one makes sure that the map never ends up in a broken "illegal matrix" state.

[anandthakker]

Thanks for the PR @hyperknot (and sorry for the delay in responding to #6782).

My concern is that this fix will hide the underlying errors that result in zoom being set to a NaN/non-number value. I'd be more in favor of making the check in set zoom() into an assert, i.e. assert(typeof zoom === 'number' && isFinite(zoom)), and fixing the root causes you identified in #6782 (comment)

[hyperknot]

Yes, I agree that the underlying cause needs to be fixed, but I wanted to make sure that the map is never frozen for any user. I'm running a custom build just for this reason.

How does a failed assert in core look for users? Everything keeps working and it's just an error in console? If so I'm happy to change the code.

[anandthakker]

A failed assert would be equivalent to throwing an exception, so no, everything would not keep working -- but it would mean that "failed to invert matrix" would be replaced with a more precise error, thus making future bugs of this sort quicker to diagnose and fix.

I agree that the map shouldn't be frozen for any user, but I'm still not convinced that placing a blanket guard in set zoom() is a good idea. It could hide other existing bugs or allow new ones to be introduced, making them subtler and more difficult to find.

The particular error from the ScrollZoom handler should really have been caught by Flow. It wasn't, because _targetZoom is incorrectly typed as number instead of ?number:

mapbox-gl-js/src/ui/handler/scroll_zoom.js

Line 47 in b9e32bf

_targetZoom: number;

[hyperknot]

Then it's up to you how do you see the timeline of properly fixing the handler bugs. My solution would be a temporary fix for user experience, until everything is polished out in scroll_zoom.

[anandthakker]

Fixed in #6924

Thanks for helping to move this forward, @hyperknot !

[hyperknot]

@anandthakker so what's with transform.js? I feel it's important to put the guard in there as well.