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

Introduce "await map.until(event)" syntax for easier async #10192

Closed
mourner opened this issue Dec 11, 2020 · 10 comments · Fixed by #10203
Closed

Introduce "await map.until(event)" syntax for easier async #10192

mourner opened this issue Dec 11, 2020 · 10 comments · Fixed by #10203
Assignees
Labels

Comments

@mourner
Copy link
Member

mourner commented Dec 11, 2020

Now that we no longer support IE11, we can take advantage of newer browser capabilities such as native promises and async/await to make programming GL JS easier. To start doing so in a backwards-compatible way, I propose adding a method to Evented-inherited classes such as Map that's just a shortcut to new Promise(resolve => this.once(event, resolve)).

This is a very tiny change, but can make the ergonomics of setting up multi-step animations and similar things much easier — instead of a bunch of nested callbacks, you would be able to write code linearly like this:

const map = new mapboxgl.Map(...);
await map.until('load');

map.addLayer(...);
await map.until('idle');

map.flyTo(...);
await map.until('moveend');
...

I explored a similar style of programming in my video export example #10172, and it feels awesome, so how does everyone feel about adding it? In the future, we might consider a full overhaul of the API (discussed in #3964) to make all async methods return a promise, but that's a substantial effort and also significantly breaking. But we could get halfway there in terms of ergonomics with a one-line change.

Alternative names:

await map.until('load');
await map.fires('load');
await map.oncePromise('load');

cc @stevage @andrewharvey

@mourner mourner self-assigned this Dec 11, 2020
@stevage
Copy link
Contributor

stevage commented Dec 12, 2020

First, yep, a promise interface would be extremely welcome.

I'm a little bit surprised that this would be a "breaking" change: await map.once("load"). I thought I had seen other libraries make similar changes - they examine their arguments and if there is no callback provided, they return a promise.

It also isn't obvious to me that making once return a promise requires making every function return a promise. There is clearly a lot of value in this specific one. (I'm not even sure how promises would apply to functions like on, off, etc).

However, if that's the case, fine. I have one more name suggestion

await map.onceAsync('load');

I don't feel very strongly about the name choices. I maybe marginally prefer either oncePromise or onceAsync. Even though they're a bit more ungainly, it's clearer that they're just variations of the same function, not something different altogether.

@stevage
Copy link
Contributor

stevage commented Dec 12, 2020

Slight further comment on naming:

If the long term plan is for await map.once(...) to be possible (once breaking change is allowed), then I'd vote oncePromise or onceAsync.

If that isn't going to happen, maybe until is better, and map.once will just fade into disuse.

@mourner
Copy link
Member Author

mourner commented Dec 12, 2020

I'm a little bit surprised that this would be a "breaking" change: await map.once("load"). I thought I had seen other libraries make similar changes - they examine their arguments and if there is no callback provided, they return a promise.

This is actually a very interesting suggestion — somehow I haven't considered conditionally returning a promise if the callback isn't provided. This would definitely not be a breaking change, and ergonomically makes a lot of sense.

The only concern is that we can't do that for similar methods like on, which could make the behavior surprising / inconsistent, but it seems like the ergonomics win outweighs the concerns to make an exception specifically for once.

@korywka
Copy link
Contributor

korywka commented Dec 12, 2020

How about:

await new mapboxgl.Map(...);
await map.addLayer(...);
await map.flyTo(...);

and yes, as @stevage mentioned above, these methods can return a promise if there is no callback passed (e.g. loadImage)

@mourner
Copy link
Member Author

mourner commented Dec 13, 2020

@Bravecow the ones you listed would be breaking changes, which we're considering but for some later time, together with a bigger overhaul of the API. For now, I'd like to find a non-breaking middle ground.

@stevage
Copy link
Contributor

stevage commented Dec 13, 2020

Also, I'm not sure that await new mapboxgl.Map() is even legal, is it? Surely the constructor has to return the object, not a promise.

And it's not clear what the semantics would be in any case - wait until the map is completely loaded, or just until it's safe to add layers (which is what users normally want).

Similarly with addLayer - is it wait until the new layer is fully rendered? Or the data is loaded? Or...what exactly?

@korywka
Copy link
Contributor

korywka commented Dec 13, 2020

  1. Yes, in real-world example it would be someting like const map = await (new mapboxgl.Map()).load();
  2. As you said, normally users want to know when it is safe to add layers. So this is when the promise is resolved. For other rare cases, events map.on(...) can be used.
  3. map.getLayer do not return undefined. So you are able to interact with it. I do not know if mapbox rendering can get a benefit, but you can render multiple layers at once await Promise.all([map.addLayer(...), map.addLayer(...), ...])

@stevage
Copy link
Contributor

stevage commented Dec 14, 2020

Maybe this would be a nice way to sidestep the giant mess around events and knowing when it is safe to add a layer, by introducing:

await map.ready()

which returns a promise which returns when you can add a layer.

(This is a bit of a tangent from the actual issue here though.)

@mourner
Copy link
Member Author

mourner commented Dec 14, 2020

@stevage yeah, I've raised this topic recently in #10191 — we should see if there's a way to allow adding sources / layers without necessarily waiting for anything.

@ryanhamley
Copy link
Contributor

I like the idea of allowing once to be flexible so that developers can do await map.once('load'); as well as map.once('load', function () {...}); As discussed above, I don't think that should be difficult to implement. I also don't think it would be especially confusing if we only added this to once because it's not obvious what await map.on() or await map.off() should really do. Expanding the API surface to add a new method just to handle the Promisified version seems unnecessary.

In conjunction with #10191, developers should have a much simpler and more modern code style for implementing maps. #10191 could even obviate much of the need for listening to load events, but even if that were to be implemented, allowing once to return a Promise for other events would be very useful. I'm 👍 on this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants