v-mapbox
v-mapbox copied to clipboard
Not accurate prop type for Marker component
In the current code the type of coordinates prop is set to Array and this is not correct. Why?
Mapbox-gl is accepting for marker coordinates a LngLatLike object, which is described as ‘an array of two numbers representing longitude and latitude, or an object with lng and lat or lon and lat properties.’ (cited from https://docs.mapbox.com/mapbox-gl-js/api/geography/#lnglatlike)
That mean that v-mapbox code is a far more restricting than the library it is using. This forces us, developers, to convert an object to an array, i.e. to use more primitive attributes than can be used. Especially in the case of longitude and latitude it is error prone – you can easily misorder the coordinates and use [latitude, longitude] instead of [longitude, latitude] (I’ve almost trained myself to check that other location immediately when my markers are not present – then started to use objects instead of arrays every time it is possible :-) ).
I'm not sure about this. We should support both if we can. When it comes to working with LngLat, it's easy to mix it up. Especially if you wrote application logic that uses the other order arround.
At the same time, I dont think it makes sense to send in an object as a prop. Instead, it would make more sense to send two props, lat and long, and have prop validation that ensures both are provided.
@Maker-Mark , the point is that mapgox-gl is already accepting LngLatLike object and also v-mapbox is working correctly passing an LngLatLike object. The only issue is that an error message in shown in the developer console – ‘you are passing an object, but an array is expected’, etc.
An array with two numbers, longitude and latitude, is also a valid value for LngLatLike object. See the source code
export type LngLatLike = LngLat | {lng: number, lat: number} | {lon: number, lat: number} | [number, number];
(copied from here: https://github.com/mapbox/mapbox-gl-js/blob/50adf1cc14e5aef09099f15c5cb803eaa5f72a48/src/geo/lng_lat.js – repository at version 1.13.1)
Is this in harmony with your words ‘we should support both’?
@karol-bujacek Ahh you're right. We just need to expect a lat lon like type. Not sure where that would go in this repo.
Current version of VMarker is written corectly: https://github.com/geospoc/v-mapbox/blob/main/types/markers/VMarker.vue.d.ts#L18