Better error messages

[acharyasourav]

Launch Checklist

• briefly describe the changes in this PR
• include before/after visuals or gifs if this PR includes visual changes
• write tests for all new functionality
• document any changes to public APIs
• post benchmark scores
• manually test the debug page
• tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
• tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
• apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

[CLAassistant]

All committers have signed the CLA.

[acharyasourav]

Hey @SnailBones What and where to make a change in the changelog? Also is spacing an issue bc while saving the code vscode made some automated spaces in the javascript?

[SnailBones]

Thank you for your work on this @Sugarlust!

Also is spacing an issue bc while saving the code vscode made some automated spaces in the javascript?

The spacing changes in camera.jssuggest that you ran a linter on this file (maybe prettier or beautify?) We use ESLint for linting (That's why the lint test is failing. You can run this test locally with yarn lint).

Can you please isolate the relevant changes without the formatting changes to the file?

[SnailBones]

Hey @SnailBones What and where to make a change in the changelog?

The changelog goes in the last entry of the Launch Checklist above. For instance:

• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Improve error handling of incorrect inputs for map.zoomTo()andmap.flyTo() </changelog>

[acharyasourav]

@SnailBones I isolated the changes. Does it seem fine now?

[SnailBones]

@SnailBones I isolated the changes. Does it seem fine now?

Thanks for doing that @Sugarlust, that's a huge improvement!

Looking at your code, I think that your checks probably aren't doing what you think they are. Can you verify the behavior works as intended? (run yarn start-debug and then you can call the functions in console.)

Lastly, it would be good to add a unit test in map.test.js to ensure that this behavior doesn't break on future changes. Here's an example of a unit test checking an error message:

mapbox-gl-js/test/unit/ui/map.test.js

Lines 1957 to 1964 in 04a4447

	t.test('throws error if invalid projection name is supplied', (t) => {
	const map = createMap(t);
	map.on('error', ({error}) => {
	t.match(error.message, /Invalid projection name: fakeProj/);
	t.end();
	});
	t.end();
	});

[acharyasourav]

@SnailBones I made the change in the error message in zoomTo. I don't really know how to write unit tests but I'm surely gonna give it a try.

[acharyasourav]

@SnailBones can you tell me how do I run the functions on console. What do I do after running yarn start-debug in the terminal. Should I fire this command directly or do I need to do something before?

[SnailBones]

@SnailBones can you tell me how do I run the functions on console. What do I do after running yarn start-debug in the terminal. Should I fire this command directly or do I need to do something before?

Instructions are on the contributing page:

Open the debug page at http://localhost:9966/debug

Then you can open the debug console with F12 and call the functions there.

[acharyasourav]

@SnailBones Idk why but I'm not able to call the flyTo and zoomTo function. It says "undefined". also correct me if I'm wrong but to call a function we call it like "map.functionName()" right?

[acharyasourav]

@SnailBones should I just write the tests?

[SnailBones]

@SnailBones Idk why but I'm not able to call the flyTo and zoomTo function. It says "undefined". also correct me if I'm wrong but to call a function we call it like "map.functionName()" right?

@Sugarlust Yes, I'm able to reproduce this issue with map.ZzomTo([0, 0]). I'm not why that isn't working for you I'd need more detail to know what's happening here.

@SnailBones should I just write the tests?

Sure! Though this may be easier if you can test the behavior in console.

[SnailBones]

Suggested change

	`zoom requires a number not ${zoom}`)));
	`map.zoomTo() requires a number in the first argument. ${typeof zoom} provided`)));

[karimnaaji]

Thanks for your time and contribution @Sugarlust but I'm not sure we should move it forward or go ahead with merging this PR.

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.

Though, I don't have an answer to improving developer experience on that regards. Maybe @mourner can weigh in as he knows more about this and could tell whether it's possible to leverage type safety across our API boundaries.

cc @SnailBones

[karimnaaji]

Also referring to #9485 as it's a similar topic and something that could be researched and considered as part of addressing error handling improvements.