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

Rename load event to ready, add ready() method #10148

Closed
comatory opened this issue Dec 4, 2020 · 2 comments
Closed

Rename load event to ready, add ready() method #10148

comatory opened this issue Dec 4, 2020 · 2 comments

Comments

@comatory
Copy link

comatory commented Dec 4, 2020

Motivation

I don't think using terminology such as load is clear enough. Me, and most likely other developers, wrongly assume that load event is fired multiple times during the lifecycle of map object. I think the confusion stems from the event named load while the method load() can return true or false as the map sources and styles are loaded.

Even I spent bunch of time figuring out why my code is not working correctly because I was relying on load() method returning either true or false and not changing to different value later on.

For my projects, I usually do something like this:

const map = new mapboxgl.Map({ ... })
map._isReady = false
map.ready = () => map._isReady
map.once('load', () => map._isReady = true))

And then in my application code I end up using:

map.ready() ? map.addLayer({ ... }) : map.once('load', () => map.addLayer({ ... }))

Design Alternatives

See my solution above. I'd imagine I could just use map.ready() and rely on it that it switches to true once the resources are downloaded and then never changes back. Then instead of using load event I'd do:

if (map.ready()) {
  // do something
} else {
  map.on('ready', () => // do something else )
}

Design

The design that I drafted above. This would probably mean getting away with load event or perhaps refashioning it in different way.

Mock-Up

Update documentation on explaining what each event and methods related to lifecycle of map object do. I don't think it's sufficiently explained in the documentation, or perhapsed I missed it.

It'd be great to have article with some examples in API documentation.

Concepts

Teaching by using examples.

Implementation

See above

@ryanhamley
Copy link
Contributor

This would be a significant breaking change which would necessitate a major version upgrade of the library. I think it's unlikely we'd do that just to rename an event/method. I agree though that the current behavior and documentation can be confusing. Perhaps a better approach that would allow us to maintain backwards-compatibility of the API would be to make the events architecture more comprehensive (adding new events, ensuring that all existing events behave the way they're expected to, etc) and making the documentation more robust and clear. This is something we're interested in pursuing in the medium-term but there's no clear timeline right now for it. Thanks for the thoughtful input though.

@comatory
Copy link
Author

comatory commented Dec 4, 2020

OK maybe just even improving the documentation would help a ton I think.

You are right that this would break compatibility. So maybe adding ready event and method as I explained and keeping the existing behaviour of load event and methods could be a way forward. Using ready could then be recommended approached for checking if Mapbox has initialized.

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

No branches or pull requests

2 participants