Reconsider popup / overlay API

[lucaswoj]

Before we ship #2725 in a release, we have an opportunity to reconsider our API design.

I would love to see an API for popups and overlays that is:

• simple
• idiomatic
• declarative
• stateless

Here's a strawdog proposal:

var overlay = map.addOverlay({
  element: document.createElement(...),
  coordinate: [lng, lat]
});

cc @mourner @jfirebaugh @tmcw @mollymerp

[jfirebaugh]

I'd prefer an API pattern that doesn't necessitate adding a method to Map whenever you invent a new thing that can be added. That's not very friendly for plugins:

• Requires monkey patching
• Map becomes a shared namespace with potential for collisions

[lucaswoj]

Yes, if we are going to release this as a plugin, the method should not be Map#addOvery, it should be a standalone thing.

var overlay = addOverlay(map, {
  element: document.createElement(...),
  coordinate: [lng, lat]
});

If this is going to be in GL JS core, is Map#addOverlay less amenable to composition and more prone to monkeypatching than a Map#overlay factory function?

[jfirebaugh]

Both new mbgl.Overlay().addTo(map) and the factory variant mbgl.overlay().addTo(map) seem like familiar patterns that have been working well in practice in both gl-js and Leaflet. What's prompting us to seek other alternatives?

[tmcw]

This does make overlay addition more similar to layer and source addition, but adding a layer or source returns this, the map, not the layer instance, and this would return the overlay instance.

It also makes the API much more similar to the mobile equivalents cc @1ec5

On the bright side, I could see this enabling something like map.getOverlays() and map.getOverlayById, basically making overlays much more similar to layers. But those APIs could be built on top of the existing system fairly easily.

So, not opposed, but not convinced that this change fundamentally improves the API.

[lucaswoj]

This does make overlay addition more similar to layer and source addition, but adding a layer or source returns this, the map, not the layer instance, and this would return the overlay instance.

Having Map#addOverlay return this is a good idea!

map.addOverlay({
  id: 'mapbox',
  element: document.createElement(...),
  coordinate: [lng, lat]
});

map.removeOverlay('mapbox')

[lucaswoj]

What's prompting us to seek other alternatives?
So, not opposed, but not convinced that this change fundamentally improves the API.

The overlay / popup API is more complicated than necessary. Reducing the API surface (hiding the Popup class, removing the Popup#addTo step, removing the concept of a detached popup, etc) makes it easier to learn and less prone to bugs.

Why refactor it now? Because over the next year we're working towards a v1 API that we're comfortable supporting "forever." Whenever there's an opportunity to substantially improve or simplify a v0 API, we should take it.

[jfirebaugh]

We shouldn't remove the concept of a detached popup; it is useful and key to a number of examples. I suspect the same will be true of overlay.

[lucaswoj]

Do you have any particular example in mind? Here's a mockup of popup-on-hover example using the proposed Popup API. Note the reduction in lines of code and the reduced need for explanatory comments.

[jfirebaugh]

Yes, I strongly prefer the existing API to that proposal. That proposal:

• Requires that Map have special knowledge of popups
• Increases the (already overlarge) API surface area of Map
• Requires that the user assign a unique ID to each popup
• Removes the ability to adjust a single property of the popup (location or contents) without re-specifying the entire set of parameters
• Is less efficient, as it requires recreating JS objects and the DOM container every time the popup is shown
• Will be less recognizable to users looking for a moral equivalent of L.Marker
• Doesn't provide a means of binding events to individual components (Add a "popup.close" event #2584), and maintaining those bindings even as the component is removed and re-added

I don't think making classes like Popup and Overlay stateless and hidden from the public API is the right approach. Users expect these to be separate class, and they expect them to have state -- and for good reasons.

[mourner]

I have to agree with @jfirebaugh here. As someone who maintained Leaflet for 5 years, I strongly prefer the pattern of the existing API.

[1ec5]

It also makes the API much more similar to the mobile equivalents

The native SDKs (particularly the OS X macOS SDK) require fewer steps than either of the above proposals to create a bare-bones popup (callout, popover, or InfoWindow, depending on the platform). But that’s mainly because the native SDKs set up basic gesture recognizers for the developer instead of requiring an explicit feature query. Think of it as the annotation declaring its hit target upfront.

I’m ambivalent about combining the overlay and popup interfaces together. Leaving the two separate would enable some markerless use cases that are quite rare on iOS, though I think you could also satisfy these use cases by supporting some sort of “empty” marker that still has a popup.

Whatever the case, my inclination would be to keep the overlay/popup interfaces separate from Map. I think we should always fight the urge to centralize functionality in Map, to keep that object’s mission clear. I also second @jfirebaugh’s concern about popups being atomic (making it impossible to set an individual property).

[lucaswoj]

Thanks for the feedback everyone! I feel fortunate to have so much experience to draw upon 🙇.

Threads of Thought

There are a few threads of thought emerging here:

Atomic / Stateless Popups 👍

There is consensus (including by me) that popups should allow non-atomic mutations and not pretend to be stateless.

Leaflet-Compatible Popups 👍

There is benefit to making GL JS popups a moral equivalent of L.Marker. This is an important consideration.

Map-Popup Coupling 💭

I think we should always fight the urge to centralize functionality in Map, to keep that object’s mission clear.

GL JS UI elements fall into two categories:

• those that are "within" the map, move with the map, for example Popup
• those that are "not within" the map, don't move the map, for example the Navigation control

What is Map's mission? I see it as the unified interface for interacting with everything that appears "within" the map. Popup and Overlay stick out as the only UI elements that are conceptually "within" the map but whose APIs are not on Map's interface.

I don't foresee a need to create additional plugins for UI elements that appear "within" the map. Overlay is meant to be a base abstraction for all of those.

If we want to simplify Map, we should simplify Map #2741.

Detached Popups 💭

We shouldn't remove the concept of a detached popup; it is useful and key to a number of examples. I suspect the same will be true of overlay.

I don't see detached popups as necessary for any of our examples. Most of them would be more straightforward and robust if they didn't use detached popups.

Detached popups been the root of bugs in the past and will likely be the root of more bugs in the future. #2395 #1811 #1477

Fewer concepts make GL JS easier to learn. Fewer imperative APIs make GL JS easier to use.

Next Steps

... will be considered after the WWDC party 😄

[lucaswoj]

Closing this in favor of incremental improvements