mapbox / mapbox-gl-js

Interactive, thoroughly customizable maps in the browser, powered by vector tiles and WebGL
https://docs.mapbox.com/mapbox-gl-js/
Other
11.09k stars 2.21k forks source link

Return promises and/or accept callbacks for all async methods #3964

Open stevage opened 7 years ago

stevage commented 7 years ago

Most of the maps I'm working on at the moment have the basic pattern:

But you can't add stuff to the map until it's fully loaded. So you either:

1: Wait for the map to load, then add the source by URL. This is slow, and misses out on the opportunity to do the two network requests in parallel.

  1. Fetch the GeoJSON yourself (in parallel), and then add it when the map is ready. You probably end up adding D3-request or similar. Then you end up with this annoying code:
        if (map.loaded())
            map.addSource(...);
        else
            map.on('load', () => map.addSource(...));

Is there an inherent reason map can't accept new sources while it's loading?

averas commented 7 years ago

We just discussed this in #2792. Personally I believe there should be at least two well defined states that are exposed via the API and signalled via events, but perhaps three:

  1. Map#ready (or something along that line...) A state the map enters as soon as it is safe to start mutating sources/layers/controls, even if the map still is busy with downloading tiles and resources. Typically as soon as the style is successfully parsed.

  2. Map#loaded A state the map enters as soon as all necessary resources and assets, such as tiles are loaded. At this point the map is at rest.

  3. Map#moving A state the map enters during transitions where it would make sense to hold off API calls that would interfere with user experience.

As elaborated a bit on in #2792 it might be tricky (and meaningless?) to distinguish between 2 and 3 since transitions inherently triggers tile fetching which also means that it is loading.

stevage commented 7 years ago

Ah, thanks. Yeah, basically I'd like to be able to add sources after Map#ready, to trigger fetching, even if they don't make it into the current render cycle.

Also, any way to clean up the is (map.loaded())... logic would be nice. Would it completely break convention to do either of these:

lucaswoj commented 7 years ago

Renamed this ticket to focus on the underlying challenge:

Make it easier to defer API calls until after the style loads

This is a real problem that deserves a good solution. Some ideas:

tmcw commented 7 years ago

Some thoughts here:

Error handling would have to be async and might get harder in a bad way

Allowing access to more methods before the map's style loads would require rethought error handling: for instance, if you allow someone to call map.addLayer('foo') immediately, and then the style arrives with a layer named foo, then what would previously be an immediate exception thrown for the duplicate ID would instead appear later on, or maybe would get emitted on .on('error'.

The proposals of .on('load', fn, true) and .onLoad are promises

.on('load' is event syntax, and follows Node's expectations of events. The proposed addition of an argument or a onLoad method follows all of the conventions of a Promise, which also resolves either async or 'immediately' if the result is already available (immediately often means next tick there).

stevage commented 7 years ago

Hmm, good points. I'm used to working with promises, so that seemed natural to me - less familiar with .on('error') patterns. The promise pattern that combines your two comments would be:

map.addLayer('foo').catch(e => {
// oops, name collision
})

This situation actually seems a bit messy even in promise-land: map.load().then(x => map.addLayer()) misses the opportunity to run the two network requests in parallel, but Promise.all([map.load(), map.addLayer()]) ends up with a non-deterministic layer ordering. Eep.

lucaswoj commented 7 years ago

Related issues: https://github.com/mapbox/mapbox-gl-js/issues/4061, https://github.com/mapbox/mapbox-gl-js/issues/1794

lucaswoj commented 7 years ago

Methods that are candidates:

Methods that are candidates but will be replaced by the unified #setCamera #3583 method:

gertcuykens commented 7 years ago

The only thing I want to add is don't bother with callbacks so you don't need to make the api more complicated than necessary. Using callbacks instead of promises should be avoided unless somebody can convince the majority, which I hope that doesn't happen :) There are plenty of polyfils for promises if a user wants to go back to the stone age :P

fergardi commented 5 years ago

Any progress on this one? How can I know when my panBy or easeTo are finished? Can we use promises now?

mourner commented 5 years ago

How can I know when my panBy or easeTo are finished

You can subscribe to the moveend event:

map.easeTo(...).once('moveend', () => { ... });
soal commented 5 years ago

I've created a library for this case: map-promisified. How it works: It wraps asynchronous methods with functions that return promises. Internally, when you call promisified method, it passes a unique event id in eventData map method argument and listens to all events that methods cause. A unique id is necessary to find out that the event is caused by certain map method. After all events with registered unique id are fired, the function returns promise.

I have two questions concerning this issue:

  1. Can I use the idle event for this purpose? Specifically, does idle fired when all transitions caused by, for example, panTo is finished?
  2. Can this approach be used in mapbox-gl-js internally?

Thank you!

ryanhamley commented 5 years ago

@soal The idle event is fired when

All requested tiles are loaded No transitions are in progress No camera animations are in progress

So yes, it should fire once all panTo transitions are finished. The original PR is https://github.com/mapbox/mapbox-gl-js/pull/7625. Adding promises to the library is not on our roadmap.

barraponto commented 4 years ago

I wanted to be able to say something like map.addLayer(...). then((layer) => {...}). But I would settle for an event fired when a layer is added -- regardless of whether it needed to load a source from a url or not.

asheemmamoowala commented 4 years ago

@barraponto This ticket is tracking the first part of your request.

I would settle for an event fired when a layer is added -- regardless of whether it needed to load a source from a url or not.

Could you open a separate tickets for the above. With some additional information on how you would expect this to work and how you would like to use it.

backspaces commented 4 years ago

I've been using:

function mapLoad(map) {
    return new Promise((resolve, reject) => {
        map.on('load', () => resolve())
    })
}

Which seems to be working. Adding a tag after the map would make it general for all map events. Is there any reason this sort of thing would fail in mapbox? Tnx!

fc commented 4 years ago

Any idea when the setFilter promise may be on the roadmap?

The current hack I'm using is a combination of using the idle and render events which works about 90-95% of the time. But, the problem is that one of the layers I'm filtering is transparent which doesn't seem to trigger the render events but I need to query the filtered transparent layers while the map is loading. I can add an extra 1 second delay but it seems there's no guarantee or way for me to know if setFilter has been truly completed.

is there any other ways to figure out if filtering is completed? I tried inspecting some of the private member variables but wasn't super clear.