Functions should throw exception on invalid args

[johnd0e]

In general I believe that it's better when functions called with invalid argument throw early, because such approach leaves less space for bugs.

Question is how far should we go implementing this, considering that we have no builtin facilities like languages like TypeScript have.

For example see #7125.

• I'm sure that more strict conditions (introduced there) are good.
  There is no explicit type checking, but code is composed with type-checking in mind, and intentionally fails on many cases of invalid args.
• But should we put that to the test (like done there)? I have some doubts, because that "fails" are not in specs, so they are specific to current implementation.
  Anyway, it should not be a problem to fix (or even remove) tests when we will need that.
• Still things are not consistent now: not all cases of invalid args are covered. E.g. DomEvent.off doesn't fail if pass invalid object instead of el. And (in other functions) event argument is not validated at all.
  All such cases could be covered with explicit type checking, but I suppose that would be overkill.

Thoughts?

[johnd0e]

After discussion #7397 we've found that toPoint is error prone, as it allows to create Point with undefined coordinates.
Particularly important case is when point is created indirectly, by some option, which can be wrongly specified as plain value like 50, instead of array [50, 50].

I believe that it's very important to catch such errors early, but we may also consider possible performance penalties.
Perhaps we should measure cost of additional checks on some large database.
And even if significant performance loss will be found, we might apply some compromise solution, like adding those checks to toPoint (instead of Point constructor).

[daverayment]

When a single non-point and non-object argument is passed to toPoint, it will end up on the last line of that function, which calls the Point constructor, which is expecting a y parameter (and optionally a round value).

return new Point(x, y, round);

In our problem case of a single number being passed, the Point constructor will therefore end up setting y to undefined:

this.y = (round ? Math.round(y) : y);

A check in toPoint for the existence of a y value would seem a good first step then. What do you think? For example:

if (y === undefined || y === null) {
    throw new Error('toPoint: Y coordinate must be defined.');
}
return new Point(x, y, round);

[johnd0e]

If you want to implement this yourself then better discuss all details in new PR.

This issue is created to discuss general questions, e.g. how far should we go in arguments validation.
Should we cover as more functions/methods/constructors as we can?
What of them are error prone (more often misused)?
How should we avoid (potential) performance loss?

1. One possible approach is adding validation to factories (e.g. toPoint()) but not to constructors (new Point()), thus leaving space for custom optimization. E.g. when you need to process huge dataset - you may prefer Point.
2. But it's not clear to me if such optimization is ever needed. To understand this we should compare real performance wit/without arguments validation. And it may appear that difference is miserable.

[johnd0e]

Some functions are protected with such code:

Leaflet/src/geo/LatLng.js

Lines 30 to 37 in 436430d

	export function LatLng(lat, lng, alt) {
	if (isNaN(lat) || isNaN(lng)) {
	throw new Error('Invalid LatLng object: (' + lat + ', ' + lng + ')');
	}
	// @property lat: Number
	// Latitude in degrees
	this.lat = +lat;

Unfortunately, this allows even weirdest coercion cases, turning '' and boolean values to valid number.

And also many places using isNaN without following explicit number conversion, like here:

Leaflet/src/layer/vector/Circle.js

Line 35 in 436430d

if (isNaN(this.options.radius)) { throw new Error('Circle radius cannot be NaN'); }

Consider case options.radius: '5', and then radius = radius + 5 - resulting in '55'.

---

Also #7761

[Falke-Design]

There is also a problem with Tiles. If the maxNativeZoom is set with a string value, the tile-request contains sometimes a high zoom value:

Leaflet/src/layer/tile/TileLayer.js

Lines 209 to 220 in d6d6051

	_getZoomForUrl: function () {
	var zoom = this._tileZoom,
	maxZoom = this.options.maxZoom,
	zoomReverse = this.options.zoomReverse,
	zoomOffset = this.options.zoomOffset;
	if (zoomReverse) {
	zoom = maxZoom - zoom;
	}
	return zoom + zoomOffset;
	},

this._tileZoom / zoom is then for example "17" and zoomOffset which is 0 is added to it, which returns 170. If a tile is requested with zoom 170, the browser window breaks / stops running. But I didn't found out why this happens