Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
stevage opened this issue Jan 12, 2017 · 12 comments

Comments

Projects
None yet
10 participants
@stevage
Copy link
Contributor

commented Jan 12, 2017

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

  • load a basemap from Mapbox (either a standard one, or slightly tweaked)
  • add stuff to it with Mapbox-GL-JS, such as a GeoJSON specified by URL

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

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2017

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:

  • map.on('load', f) immediately calls f if the map is already ready.
  • map.on('load',f, true) the same, with a flag that defaults to false for current behaviour.

@lucaswoj lucaswoj changed the title Allow adding sources while loading Make it easier to defer API calls until after the style loads Jan 17, 2017

@lucaswoj

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2017

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:

  • Allow any API call before the style loads and transparently delay their action until after the style loads
  • Provide a new Map#onLoad(callback) API which runs callback immediately if the map has loaded or after the style loads if not
  • Add more granular Map#isStyleLoaded-like methods
@tmcw

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2017

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 lucaswoj changed the title Make it easier to defer API calls until after the style loads Return promises and/or accept callbacks for all async methods Jan 30, 2017

@lucaswoj

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2017

Related issues: #4061, #1794

@lucaswoj

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2017

Methods that are candidates:

  • GeoJSONSouce#setData
  • Map#addLayer
  • Map#addSource
  • Map#fitBounds
  • Map#loaded (refactor to be a promise that is fufilled when the map has loaded initially, perhaps rename)
  • Map#moveLayer
  • Map#remove
  • Map#removeLayer
  • Map#removeSource
  • Map#setFilter
  • Map#setLayerZoomRange
  • Map#setLayoutProperty
  • Map#setPaintProperty
  • Map#setStyle

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

  • Map#easeTo
  • Map#flyTo
  • Map#jumpTo
  • Map#panBy
  • Map#rotateTo (?)
  • Map#setBearing (?)
  • Map#setZoom (?)
  • Map#zoomIn
  • Map#zoomOut
  • Map#zoomTo
  • Map#zoomTo
@gertcuykens

This comment has been minimized.

Copy link

commented Jan 31, 2017

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

This comment has been minimized.

Copy link

commented Mar 20, 2019

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

@mourner

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

How can I know when my panBy or easeTo are finished

You can subscribe to the moveend event:

map.easeTo(...).once('moveend', () => { ... });
@soal

This comment has been minimized.

Copy link

commented Mar 20, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@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 #7625. Adding promises to the library is not on our roadmap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.