-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Enforce LngLatLike param in LngLat.convert #3232
Conversation
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.
Looks good. As far as I remember, the only reason for pass-through was to handle undefined
/null
values to enable things like var value = LngLat.convert(lnglat) || defaultValue
, but I'm not sure whether it's used anywhere now.
@@ -82,9 +82,11 @@ LngLat.prototype.toString = function () { | |||
LngLat.convert = function (input) { | |||
if (input instanceof LngLat) { | |||
return input; | |||
} | |||
if (Array.isArray(input)) { | |||
} else if (input && input.lng && input.lat) { |
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 check is dodgy: 0, 0
is a valid point but this will fail since 0
is falsy
} | ||
if (Array.isArray(input)) { | ||
} else if (input && input.lng && input.lat) { | ||
return input; |
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.
If given {lng, lat}
object, it should return a LngLat
instance rather than the object.
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.
As far as I remember, the only reason for pass-through was to handle undefined/null values to enable things like
var value = LngLat.convert(lnglat) || defaultValue
, but I'm not sure whether it's used anywhere now.
I know one place it is still used: http://github.com/mapbox/mapbox-gl-js/blob/master/js/geo/lng_lat_bounds.js#L83-L84
We will need to fix that site and check for others.
* stricter object property checking + LngLat conversion * don't rely on passthrough of non-LngLatLike args in LngLatBounds * add tests for cases of concern
@@ -82,8 +82,8 @@ LngLat.prototype.toString = function () { | |||
LngLat.convert = function (input) { | |||
if (input instanceof LngLat) { | |||
return input; | |||
} else if (input && input.lng && input.lat) { | |||
return input; | |||
} else if (typeof input !== 'undefined' && input.hasOwnProperty('lng') && input.hasOwnProperty('lat')) { |
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 truthy, so no need for the first typeof. Also, I think you can safely use 'lng' in input
instead of hasOwnProperty
because it doesn't matter in this case if it's own or inherited from a prototype.
return this.extend(LngLatBounds.convert(obj)); | ||
} else if (obj.every(function(i) { return !isNaN(i); })) { | ||
return this.extend(LngLat.convert(obj)); | ||
} |
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.
Let's avoid closures here — they're not necessary and can be a perf hit.
👍 thanks for the review everybody — have addressed in a702272 and c1e423c. Good catch @lucaswoj — I fixed and went through everywhere we use
|
This PR enforces
LngLatLike
-ness of theLngLatLike
argument toLngLat.convert
.Previously
LngLat.convert
passed through non-LngLatLike
(as class instance or array) arguments unchanged. In updating tests I caught once instance where its arguments become{lng: <lng>, lat: <lat>}
but not aLngLat
class instance, so I added that conditional toLngLat.convert
. If its argument is none of those three types it now throws an error describing the nature of aLngLatLike
argument.This method is used in the internals of methods like
setCenter
,panTo
, etc etc and should stop error reports of the ever-cryptic "failed to invert matrix".@lucaswoj — there's not some special reason I'm missing why
LatLng.convert
may need to pass through non-LngLatLike arguments, is there? (I removed the one test that verified that worked: c3820ae#diff-c6936a6d2842db2080af49f8c5dac23aL27)Launch Checklist