Removed bindall, added some types, changed source.ts factory

[HarelM]

Superficial refactoring to remove bindall and introduce the typescript () => way of doing things in modern code.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (6794451) 73.81% compared to head (89a3195) 73.85%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2707      +/-   ##
==========================================
+ Coverage   73.81%   73.85%   +0.03%     
==========================================
  Files         238      238              
  Lines       18928    18916      -12     
  Branches     4244     4250       +6     
==========================================
- Hits        13972    13970       -2     
+ Misses       4956     4946      -10     

Impacted Files	Coverage Δ
src/source/canvas_source.ts	81.60% <100.00%> (ø)
src/source/geojson_source.ts	85.48% <100.00%> (ø)
src/source/image_source.ts	82.67% <100.00%> (ø)
src/source/source.ts	96.96% <100.00%> (+6.96%)	⬆️
src/source/vector_tile_source.ts	95.09% <100.00%> (ø)
src/source/video_source.ts	65.43% <100.00%> (ø)
src/style/style.ts	83.76% <100.00%> (-0.05%)	⬇️
src/ui/camera.ts	92.89% <100.00%> (-0.02%)	⬇️
src/ui/control/attribution_control.ts	100.00% <100.00%> (ø)
src/ui/control/geolocate_control.ts	77.18% <100.00%> (-0.09%)	⬇️
... and 12 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[birkskyum]

Why are arrow functions preferred in object/class methods?

[HarelM]

They preserve "this" when used with event listeners. They do the same as bindall basically.
There's is a different solution to this by adding () => when registering the events. Not sure which is better...

[rotu]

They preserve "this" when used with event listeners. They do the same as bindall basically. There's is a different solution to this by adding () => when registering the events. Not sure which is better...

I'm a fan of the method definition syntax because:

1. I find the arrow function harder to read (name = (...args) =>), and the return type before the arrow break my brain)
2. I can use super, which cannot be used with arrow function syntax.
3. It has more natural debugger support. Using the arrow definition syntax makes a property enumerable, so it shows up in debugger output where a method wouldn't:

---

The advantage of late-binding (when registering the callback) is you don't have to look at the implementing class to tell whether this is bound. You can always assume it isn't (as is the case with builtin classes). I personally prefer late-binding in general.

The problem with late-binding which can make trouble:

1. It's an implementation detail you can easily forget.
2. It's harder to unsubscribe an event listener - the caller needs to keep a reference to the bound function (similar to how you need to save the return value of setInterval if you ever want to call clearInterval).

---

Another option to consider is decorators. But I don't think we should be using them. It's still too exotic a feature.

---

It seems suspicious that many properties of Source are bound by the implementing class - that's a major gotcha that we haven't represented in the type system.

e.g. prepare is typed as () => void but, if I'm reading the code correctly, implementers must instead conform to the stricter type (this:unknown) => void.

[HarelM]

I'm not sure what you are suggestion here, feel free to submit a PR to improve what you don't like, I generally don't see the value in bindall, I also discovered places where it wasn't "correct".
If you prefer method invocation and you'd like to register the events with () => this.method() go ahead.
I'm not 100% sure the source parameters are truly those that need binding as some of them were incorrect and it might be from an old implementation...

[clementmas]

One clear disadvantage of these arrow functions is that it breaks inheritance.

Am I missing something or is it not possible to extend class methods anymore using MapLibre v3?

Edit: I need to also use arrow function to override the parent method but I can't call super() I guess.

[HarelM]

I don't like these arrow function in general, but I dislike bind more, so I think this is a lesser evil.
I prefer composition over inheritance any time, but there are complicated cases here for composition.
If you have a better way to remove these arrow function and keep the code without these bind stuff, please open a PR and I'll be happy to review it.
One option I guess would be to save the registration callbacks as private members in order to unsubscribe them, but that's a bit messy as well.
In any case, any improvement is welcomed.

[neodescis]

I agree with @clementmas. I also strongly dislike bind, but I'm not sure breaking inheritance is better? Also, I believe this is a breaking change if it breaks inheritance, especially in the controls. Breaking inheritance there seems especially onerous.

[HarelM]

Looking deeply at this PR, most of the methods are prefixed with _ meaning private, so these are not part of the public API.
I think the methods in the sources do not require the change and the bind all there was redundant, but this is a bit besides the point, and we might consider fixing it there.
There is the navigation control methods which I think should be prefixed with _ and it's probably a mistake that they are not.
Bottom line, I agree that this is a breaking change in theory, in practice it's very much contained.
Nevertheless, I'm thinking about version 4, so it might come sooner than later...

I do think we should create some helper method to allow registering an event with an anonymous method and returning something you can unsubscribe from, like rxjs does.
But this require a bit more change.
I'll write it down for version 4 breaking changes.