Fix circular dependency between lng_lat_bounds.ts and lng_lat.ts

[zhangyiatmicrosoft]

Launch Checklist

src/geo/lng_lat_bounds.ts -> src/geo/lng_lat.ts -> src/geo/lng_lat_bounds.ts

Both are public API so cannot change any signature. The problem was in toBounds method of LngLat class.

Solution:

• rename original lng_lat.ts to lng_lat_partial.ts, import type LngLatBounds only, so circular dependency is broken.

• leave only stub of toBounds

• make another file lng_lat.ts, and actually add the body of toBounds

• In lng_lat_bounds.ts only import lng_lat_partial

• Everywhere else, all references to LngLat class remain the same.

• A minor clean up on two constants to avoid future entanglement to these two classes.

• Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!

• Briefly describe the changes in this PR.

• Link to related issues.

[HarelM]

I think we can break API since we are about to release version 3.
I think toBounds should be removed and LngLatBounds should have a new (static?/constructor?) method called fromLatLng() or something similiar.
Also, LngLatBounds constructor types are just horrible, would be great to improve that.

[zhangyiatmicrosoft]

I think we can break API since we are about to release version 3. I think toBounds should be removed and LngLatBounds should have a new (static?/constructor?) method called fromLatLng() or something similiar. Also, LngLatBounds constructor types are just horrible, would be great to improve that.

Improve it in what way and to what extend? It is a quite subjective goal that can easily spin into big discussions.
I'd propose to make it a different task, whereas this one is as simple as an engineering hygiene so we can turn on warning as error as soon as possible to stop all other potential problem that might happen.
And, this PR does not stop or harm future improvements, only help them. :)

[HarelM]

The code in this proposal is very complex when the goal is to remove circular dependecies, IMHO.
Since we are talking about basically a single method to convert latlngs to bounds using a radius, I don't think "moving it a bit" is a problem.
The types of the LngLatBounds constructor are any, and the constructor assumes a lot of things about the input, which is very hard to understand from the code without the types (it's also complicated if you has the types, but less).
So I suggest the following:

1. Add types to LngLatBounds constructor.
2. remove toBounds
3. Add a method in LngLatBounds to create bounds from LngLat and radius, either in the constructor (which might complicate things even more, and I'm not sure that's what we want to do) or create a static method called fromLngLat(lnglat, radius).

I think it's a cleaner solution to this circular dependency.

[zhangyiatmicrosoft]

How much emphasis should be put into maintaining backward compatibility? Any official guidance?
Should it be put to deprecated first, and let run for a while and then break?
Even when documented, it could be a resistance to move to the new version if API changes trigger a lot of application changes.
I remember pretty often a PR is blocked because of changing public API -- or potentially might be changing it. Yet here it is encouraged to do so, and confused when to apply which rule.

[HarelM]

I would say that the main map API should be relatively stable, we shouldn't break things all the time, but since we are planning a major release we are allowed to break some things. This method is relatively internal and so I don't we should feel to bad for moving it a bit (it's not that it won't be possible after the upgrade, and typescript should help us here).

[zhangyiatmicrosoft]

replaced by #2188