L tooltip

[nmezzopera]

This PR adds support for the tooltip component

Closes #19

[nmezzopera]

@mikeu would you mind taking a look here? I have a feeling that we could do something to simplify the code.. But I really want to ditch findRealParent and replace it with provide/inject this means that we need to wrap any leaflet method that we need to pass down

Anyhow let me know what you think!

[mikeu]

@DonNicoJs for sure, I should have some time to check it out this weekend, or failing that early in the week. Looking forward to seeing what you've come up with so far!

[mikeu]

@DonNicoJs I agree, I really like the idea of replacing findRealParent with provide/inject, and so far at least it certainly seems doable. I just pushed some changes based off of this l-tooltip branch, to another branch, taking a stab at updates to your approach here.

I had a whole thing written about the changes I'd made to the map component, when I finally noticed that the changes were almost, but not quite, in line with what you'd already done in several of the other components, so I refactored my changes to follow the general scheme you already had, and here's a different version of my comments :)

I think the biggest change I've made is to explicitly provide methods individually where they are set up, and then to only inject the methods that are needed. This makes for a few more lines of code than providing and injecting the generic leafletMethods / lMethods object, but I also think that it makes it easier to work with, hopefully less error prone when a component needs to override ("overprovide"?) a method that is also provided by its parent, and also hopefully easier to work with in tests, if we want to mock the injected methods.

The only component that I didn't update in this way was the marker. I wanted to ask you about something first: What do we gain by awaiting delayed imports for the leaflet components inside the onMounted handler? It seems as if things would be a lot simpler if we just imported e.g. L.latLng where it is needed, instead of having to have an empty function as a ref that gets replaced with the Leaflet function when the component mounts, and it's not as if the map components can display themselves at all until those imports have finished anyway, so I'm curious to see what the benefit of the extra complication is.

Finally, I switched the naming of mapRef and mapObject to leafletRef and leafletObject. In Vue2Leaflet I often find it a little strange that mapRef would not always (really, almost never) actually reference a map, and I know it's caused confusion for others at least once, from a discussion on the discord. I also considered switching instead to markerRef, layerRef, circleMarkerRef, etc., depending on the component, but I like the idea of always knowing that the reference to the Leaflet elements will be called the same thing. Happy to switch back if you prefer, just thought it was a change I'd try out as early as possible in the development.

[nmezzopera]

@mikeu Thank you for the review! I've created a PR from the branch to check the differences more easily.

The only component that I didn't update in this way was the marker. I wanted to ask you about something first: What do we gain by awaiting delayed imports for the leaflet components inside the onMounted handler?

This is the main point that complicates the code, but is also crucial to achieve two things: Easier bundle splitting in ESM modules and out of the box SSR support. Leaflet is inherently not SSR compatible so importing it anywhere else beside a mounting point would preclude us to have SSR support.

This problem is compounded by few others:

• We can't call setup inside an async function / mounted hook (vue 3 emits a warming)
• If we pass a single method we can't override it later when the real one is ready (after we imported stuff from leaflet)

This 3 factors is what lead me to use a reactive object to store 'fake empty methods' that can be passed down and then re-written inside the mounted hook when they can use the underlying leaflet methods.

I thought about splitting this in different pieces, one for normal methods and one for ones that require leaflet but I think ti became more confusing.

Finally, I switched the naming of mapRef and mapObject to leafletRef and leafletObject. In Vue2Leaflet I often find it a little strange that mapRef would not always (really, almost never) actually reference a map, and I know it's caused confusion for others at least once, from a discussion on the discord. I also considered switching instead to markerRef, layerRef, circleMarkerRef, etc., depending on the component, but I like the idea of always knowing that the reference to the Leaflet elements will be called the same thing. Happy to switch back if you prefer, just thought it was a change I'd try out as early as possible in the development.

I do like this, but I will go with one name fitting all, so leafletRef and leafletObject is probably the best option in my opinion.

I like the concept of over-provide maybe we should wrap this in a utility function to enforce the meaning.

My idea would be: let's move on with this PR, let's open a new one to swap mapRef/mapObject for leafletRef/leafletObject

But First I would like to know your thoughts!

[mikeu]

This is the main point that complicates the code, but is also crucial to achieve two things: Easier bundle splitting in ESM modules and out of the box SSR support. Leaflet is inherently not SSR compatible so importing it anywhere else beside a mounting point would preclude us to have SSR support.

Ah OK, thanks! SSR is one thing I have no experience with, so I will happily defer to you on that front :)

This 3 factors is what lead me to use a reactive object to store 'fake empty methods' that can be passed down and then re-written inside the mounted hook when they can use the underlying leaflet methods.

Yeah, given that set of requirements, I'm not having any luck thinking of a better alternative either. My only suggestion at this point, which I can throw into a different PR if you like it, would be to define a function (say in utils.js) called something like leafletPlaceholder, and use that everywhere we need the fake empty methods. It could, for example, console warn that a method is being called before being replaced by the Leaflet one.

My idea would be: let's move on with this PR, let's open a new one to swap mapRef/mapObject for leafletRef/leafletObject

Sounds good to me!

[mikeu]

I like the concept of over-provide maybe we should wrap this in a utility function to enforce the meaning.

Remembered I had one other comment. I like this idea, it's similar in spirit to the leafletPlaceholder function I proposed above, but I am not certain how this one would work in practice. My main concern is that it might not be possible to know whether a function is over-providing or merely providing.

For example, in Leaflet both layer groups and tile layers have a getAttribution method. If a tile layer is used directly on a map, it will be providing that, but if it's inside a layer group then it will be over-providing it. At this point I don't know whether getAttribution in particular is a function we'll need to provide at all, but it's just the first example I thought of—it's probably also not the only function that might lead to this sort of situation.

Unless you had other ideas for the utility function, like maybe it would do some checking or console warning of some sort when in development mode, beyond simply being an explicit indication that something is being overridden on purpose? Or would it just be useful to acknowledge in the code that there is even the possibility that it's happening, and we've made that choice explicitly?

[nmezzopera]

@mikeu Thanks for all the input! So to not make this PR out of scope too much WDYT if we do like this?

• Approve this as is and merge
• open a new PR to create leafletPlaceholder util function and use it in the current code
• modify your already created PR to swap mapRef with leafletRef (and the others)

Does this sounds good to you? I see that you approved but somehow Github is not happy and still asking for your complete review! Can you take a look? Nevermind the UI was broken 😅

I'll go along and merge this, and will create a new PR for the utility! do you have time to modify your Pr/Branch to swap mapRef?