feat: update to mapbox-gl 1.12.0

[AnthonyMacKinnon]

Here is a detailed list of changes covered by this PR:

• Update mapbox-gl dependency to >= 1.12.0 in order to take advantage of latest features.
• Extract types from @types/mapbox-gl, instead of redefining them, for most inputs. This makes it more futureproof and also resolves some missing valid values.
  • For example, @Input() canvas: string; gets replaced with @Input() canvas: CanvasSourceOptions['canvas'];
• mgl-layer
  • Add missing events
    • dblClick
    • mouseDown
    • mouseUp
    • mouseOver
    • mouseOut
    • contextMenu
    • touchStart
    • touchEnd
    • touchCancel
• mgl-map
  • Add missing inputs
    • collectResourceTiming
    • crossSourceCollisions
    • fadeDuration
    • minPitch
    • maxPitch
    • touchPitch
  • Move renderWorldCopies to dynamic inputs since there is now a method to update it on the map.
  • Remove classes input since there is no matching option on the Mapbox map.
  • Use proper types for all events, except load since I didn't want to introduce a breaking change.
  • Remove mouseEnter and mouseLeave events since they only apply to a layer and don't work when none is specified.
• mgl-canvas-source
  • Updating coordinates now uses the relevant method on the map.
• mgl-geojson-source
  • Remove minZoom input since there is no matching option in Mapbox.
  • Add missing inputs
    • attribution
    • clusterMinPoints
    • lineMetrics
    • promoteId
    • filter
• mgl-raster-dem-source
  • Add support for raster DEM sources.
• mgl-raster-source
  • Add missing inputs
    • attribution
    • scheme
• mgl-vector-source
  • Add missing inputs
    • bounds
    • scheme
    • attribution
    • promoteId
  • Updating url or tiles now uses the relevant method on the map.
• mgl-video-source
  • Updating coordinates now uses the relevant method on the map.
• Enable Prettier format on save in VS Code.

There are no breaking changes since the few inputs that were removed do not have matching options in Mapbox and therefore didn't do anything as far as I can tell. Note that this PR also covers the changes proposed in #246, #248, and #252 so there is no need to merge them if this gets approved.

[AnthonyMacKinnon]

@Wykks @dmytro-gokun We really appreciate the effort that has been put into the project so far and would love to contribute. For us, this means keeping it up to date with Mapbox as it evolves.

Is there a plan to review and merge pull requests into this repository? If not, we can fork it and make future improvements there to remain current. I would, however, prefer to stick to this one since it's better for the community.

[dmytro-gokun]

@dmytro-gokun Thanks for the effort. Honestly, i saw this PR but it looked so big and a lot of unrelated changes packed into one thing. Kind of hard to review. Would you mind splitting this into unrelated PRs, one-per-feature?I'll try to review those shortly and publish an updated version.

[AnthonyMacKinnon]

@dmytro-gokun Thanks for the reply. Although there are several files, it's the same logic applied to each one:

• add missing inputs and events
• tweak calls to Mapbox where appropriate

The reason I updated the map, layer, and sources all at once is to be consistent with bumping the Mapbox version to 1.12. I listed every single change in the PR description for convenience, so it honestly looks more intimidating than it is.

Nevertheless, I can split these 18 files into separate PRs. It will likely be the same changes, just fewer files at a time. Is this your preference?

[dmytro-gokun]

@AnthonyMacKinnon Yes, I'd like this to be split to small, bite-sized PRs which can be reviewed separately. I know, it's tedious, but it's better to keep thing separate.

Question. Are all of this changes related to mapbox-gl 1.12.0? E.g., "mgl-layer.dblClick" would not work with 1.11.0 and lower, right? The reason i want to understand this is that i do not want us to require our users to update mapbox-gl if that is not really needed.

[AnthonyMacKinnon]

@dmytro-gokun Some of the changes require 1.12 (e.g. setTiles and setUrl on vector tile source, filter on GeoJSON source), some may work with 1.1, and some require a version in between. I'm not going to go through the mapbox-gl change log and source code for each version to make a dozen or more PRs here though. Going from 1.1 to 1.12 is a minor revision, so it shouldn't be a significant issue for anyone. People can also opt to stay on an older version if need be.

Since this project is a wrapper around mapbox-gl, it makes sense to keep it in sync with mapbox-gl releases. This is our goal moving forward, so future PRs will be more manageable. If this approach does not match yours, no worries. We'll simply work off a fork.

[dmytro-gokun]

@AnthonyMacKinnon

Since this project is a wrapper around mapbox-gl, it makes sense to keep it in sync with mapbox-gl releases.

I'm afraid it's a bit more complex than that. We also have Angular version as another dimension. I do not want to require ppl to use the latest mapbox just for sake of it. This is no library's business. So, we need to make sure we are not forcing ppl to update peer dependencies in a minor update (it's okay to do that when we release a major version).

If this approach does not match yours, no worries. We'll simply work off a fork.

Sure, i'm okay with any forks if that suits your workflow better. Again, if you want to contribute with more manageable PRs - you are welcome.

[AnthonyMacKinnon]

@dmytro-gokun

I'm afraid it's a bit more complex than that. We also have Angular version as another dimension.

Agreed, determining which version of Angular to target is not trivial since it's not always easy or even possible to upgrade large projects to a new version. One option could be to have concurrent versions of ngx-mapbox-gl targeting different Angular versions (e.g. 9.x, 10.x, 11.x). It's obviously more work to do so, however.

I do not want to require ppl to use the latest mapbox just for sake of it. This is no library's business. So, we need to make sure we are not forcing ppl to update peer dependencies in a minor update (it's okay to do that when we release a major version).

I'd argue that picking the mapbox-gl version is different since it is the most important dependency here. Targeting the latest version is not for the sake of it. It's to ensure ngx-mapbox-gl doesn't becomes stale. It's to allow existing projects as well as new projects to take advantage of the latest features and performance improvements. You can't do this while ngx-mapbox-gl depends on an older version. Even if my local project depends on mapbox-gl 1.12, I can't use this directive for the new inputs and events. This goes against its whole purpose.

Looking back, I see that the upgrades to Angular 10 (3976db7), Angular 8 (57f6d6e), and Mapbox GL 1.1 (1043b4f) were all done in minor revisions and included breaking changes. Targeting a new version without breaking changes, as I'm proposing here, should be a transparent process for consumers of ngx-mapbox-gl. It has no negative side effects and many positive ones.

As I wrote earlier, I'm open to splitting up this PR if it will help. I'm not sure how though. I could make one for mgl-raster-dem-source since it was missing and is not related to the new mapbox-gl version. The other changes are either to fill existing holes or to match the new version.

[Wykks]

Hi!

Although I'm not really active on ngx-mapbox-gl, I agree with the logic "if there's no breaking change, update to mapbox-gl are fine". And I even encourage them.
There may have been some "mistake" in the past, but I always tried to follow semantic versioning. So yeah, breaking changes should means major version.

But there's one annoying issue : peerDependencies. And that's even more relevant with newers version of npm. Because npm will raise an error if peerDependencies are not compatible (this is really a bad change tbh...).

I believe the most important thing for ngx-mapbox-gl is to be up to date with mapbox-gl (although I'm not really in a good position to say that, but that's what I was doing when I was actively maintaining this lib).

One option could be to have concurrent versions of ngx-mapbox-gl targeting different Angular versions (e.g. 9.x, 10.x, 11.x). It's obviously more work to do so, however.

I agree, I tried to that in the past, but welp... Can't say that's an easy task 😅

Well since I'm here, I'm going to review your PR :)
(Also the size is OK for me since changes seems to be only related to the mapbox-gl update)

[Wykks]

Really good work 👍
Some changes could indeed have been in another PR but it's ok.

Only one major change request, in the new mgl-raster-dem-source component (see comment)

Seems safe to me to be part of a minor release. But I'm also fine to make a major version out of this too, because they may be some typings breaking changes (and the mapbox-gl version gap is quite important too). What do you think @dmytro-gokun ?

[Wykks]

Missing doc here. Do github still not allow PR for Wiki ? I really should move docs to the repository...

[AnthonyMacKinnon]

Correct, I don't have the ability to edit the wiki unfortunately.

[AnthonyMacKinnon]

@Wykks

Thanks for the review! Just pushed a few changes based on the feedback.

Seems safe to me to be part of a minor release. But I'm also fine to make a major version out of this too, because they may be some typings breaking changes (and the mapbox-gl version gap is quite important too).

Just FYI, but I did go through each field to compare and the new types were only extending what was already there if I remember correctly. With that said, up to you and @dmytro-gokun to decide what makes sense from a versioning perspective.

[dmytro-gokun]

@Wykks If there are no breaking changes then there is no need for a major version. But then we also should not update peerDependency version. If we decide to update peerDependency version - that screams a new major version to me :). As far as I can see, mapbox-gl itself is updating minor version and, thus, we should not expect any breaking changes here.

[AnthonyMacKinnon]

@dmytro-gokun @Wykks Oh, interesting. I hadn't actually noticed the different package.json in the root which includes the peerDependencies. It's probably an issue if we don't bump up mapbox-gl to the same version here. There are no breaking changes in the interface itself, but new methods used to update values internally (e.g. setTiles, setUrl) won't exist in earlier versions. Therefore, those calls will fail if you're using the new version with 1.1 locally for example. The same applies to new inputs and events.

Seems to me like the peer dependency version needs to match as we target new versions. Otherwise, consumers might run into issues. Not sure I'd use a new major version though since it's likely that the same thing will happen each time we target a new mapbox-gl release (1.13, 1.14, etc.).

[dmytro-gokun]

@AnthonyMacKinnon

I hadn't actually noticed the different package.json in the root which includes the peerDependencies

Yes, we set peerDependncies when building the library itself. IMO, we should keep that peerDependency set to 1.1 as long as we do not introduce any breaking changes.

Therefore, those calls will fail if you're using the new version with 1.1 locally for example.

That is fine to me as long as it works fine with older version when new features not used. That what backward compatibility means :).

---

After some thinking. It might be easier (from the support point of view) to update peerDependncies to be >= mapbox-gl.latestVersion when we add support for a new version of mapbox-gl. However, if we do that, we must also increment majorVersion of this library if want to adhere to semantic versioning. And that is a bit problematic to me, as it means we'll end up with versions like 65 and even more pretty soon. Maybe not a real problem, though.

[AnthonyMacKinnon]

@dmytro-gokun

After some thinking. It might be easier (from the support point of view) to update peerDependncies to be >= mapbox-gl.latestVersion when we add support for a new version of mapbox-gl.

Agreed. If we don't update peerDependencies, it gives the consumer the impression that they can use an older version without issues. Putting the responsibility on them to know which calls work and which ones don't is really not great in my opinion. There are also cases where "old" inputs could no longer work because the underlying call to Mapbox has changed.

However, if we do that, we must also increment majorVersion of this library if want to adhere to semantic versioning. And that is a bit problematic to me, as it means we'll end up with versions like 65 and even more pretty soon. Maybe not a real problem, though.

Yeah, I guess we get into the semantics of semantic versioning. 😄 Personally, I don't consider having to update mapbox-gl along with ngx-mapbox-gl as a breaking change since they're entirely linked to one another. If there are breaking changes in the interface, then that's a different matter. Just my 2 cents.

[dmytro-gokun]

@AnthonyMacKinnon

There are also cases where "old" inputs could no longer work because the underlying call to Mapbox has changed.

Well, while theoretically possible, that would also mean that mapbox team have violated sematic versioning. Then we may have bigger problems than that :).

Yeah, I guess we get into the semantics of semantic versioning. 😄

Well, we are not first people to discuss that. I think, it is universally agreed that updating peer dependency version is a breaking change and, thus, should be marked as a new major version. Check this for example: semver/semver#502 I do not think it's a good idea for us to break this rue of thumb.

Again, if we want to make our lives easier and are not afraid of versions like 65 (after all, Chrome is not afraid :)), we should just go ahead updating peer dependencies versions and releasing new major versions of this package.

@Wykks What do you think?

[AnthonyMacKinnon]

There are also cases where "old" inputs could no longer work because the underlying call to Mapbox has changed.

Well, while theoretically possible, that would also mean that mapbox team have violated sematic versioning. Then we may have bigger problems than that :).

Here's one example. In the current PR, binding tiles and url on mgl-vector-source now uses setTiles and setUrl respectively. These inputs existed before, but the new way they're handled in the directive will not work if you're using mapbox-gl < 1.12.0.

Well, we are not first people to discuss that. I think, it is universally agreed that updating peer dependency version is a breaking change and, thus, should be marked as a new major version. Check this for example: semver/semver#502 I do not think it's a good idea for us to break this rue of thumb.

Very useful thread, thanks for sharing. In my mind, this applies in nearly all cases but not necessarily here. I see the dependency on mapbox-gl in a similar light to how @angular/cli or @angular-devkit/build-angular handle their dependency on Angular. Both follow Angular's versioning, albeit in a different way. @angular/cli matches the version of Angular while @angular-devkit/build-angular embeds the targeted version in its own minor version (e.g. 0.1000.3 targets 10.0).

ngx-mapbox-gl is meant to logically follow the releases of mapbox-gl. In a way, it would have been convenient if it matched its versioning. Since we're at 4.x and Mapbox is at 1.x, an alternative could be to use Angular as the major version and mapbox-gl as the minor version. For example:

• ngx-mapbox-gl 9.101.0 targets Angular 9 and mapbox-gl 1.1
• ngx-mapbox-gl 10.112.0 targets Angular 10 and mapbox-gl 1.12

[dmytro-gokun]

@AnthonyMacKinnon

ngx-mapbox-gl 9.101.0 targets Angular 9 and mapbox-gl 1.1
ngx-mapbox-gl 10.112.0 targets Angular 10 and mapbox-gl 1.12

While this is clever, it clearly violates the idea behind semantic versioning. Personally, I do not like this as it goes against the community accepted standards and tooling which implies semantic versioning. However, I'd like to @Wykks to have a final say here as he is the author of this package.

[Wykks]

ngx-mapbox-gl 9.101.0 targets Angular 9 and mapbox-gl 1.1
ngx-mapbox-gl 10.112.0 targets Angular 10 and mapbox-gl 1.12

Yeah this makes sense, but that's not practical.

Let's think about it more pragmatically.
This is a jump from mapbox-gl 1.1 to 1.12. ~1 year of active dev of mapbox-gl.
Also, as much as you tried not to, there's a small chance that this PR contains breaking changes (as stated above).

So we should just release this as part as a standard major version.

And for the upcoming version of mapbox-gl, we may, or may not, increase major version depending of how confident we are about not introducing breaking changes.

Thanks for your help ! :)

(Welp, meanwhile, mapbox-gl just released v2.0.0 🤣 )

[Wykks]

(I'm going to work in the v5 release this week for hopefully a release this week-end)