mapbox / mapbox/mapbox-gl-directions

Directions layers/sources never gets added to the map if the map styleState/sourceState is dirty

Open
#111 2 comments 2 reactions 0 assignees View on GitHub
auto-triaged bug
Dominant language
JavaScript
Stars
255
Forks
130
PR merge metrics
No merged PRs in 30d

Description

I was trying to figure out why my directions were never loaded and the layer/source didn't actually get loaded.

As it turns out, there was a race condition where despite the map itself actually having been loaded (I wasn't even adding the control to the map until after map.('load')), the `mapState` method on Directions was never called. I looked into it and the issue is that the code for this library checks `_map.loaded()` -- and if that's falsey waits for `map.on('load', mapState)`. The issue is that `map.loaded()` can return `false` even if the map has already been loaded -- it just means that there has been a change to the styling or sources. In this case `map._loaded === true` but `map.loaded() === false`. When this happens, mapbox-gl-js never fires a `load` event -- as it already has done so.

Unfortunately though, it doesn't appear like mapbox-gl-js fires any event to indicate that it has *left* this "map.loaded() === false" state, just from looking through what happens on `_rerender` and `_update` in mapbox-gl-js.

So to boil it down:

`onAdd` in this library checks to see if the map is loaded by calling `map.loaded()`, but `map.loaded` is a bit overloaded, it doesn't just mean that the map has loaded, it can also mean that it's in the middle of a rendering/animation cycle.

https://github.com/mapbox/mapbox-gl-directions/blob/master/src/directions.js#L83
https://github.com/mapbox/mapbox-gl-js/blob/master/js/ui/map.js#L1115
https://github.com/mapbox/mapbox-gl-js/blob/master/js/ui/map.js#L1171

This appears to have been introduced in: https://github.com/mapbox/mapbox-gl-directions/pull/86

For now, I fixed this in my own usage by extending MapboxDirections and overriding `onAdd` to check for this weird state. Obviously this is not a long term solution.

```
class FixedDirections extends MapboxDirections {
// hotfix for the onAdd issue
onAdd(...args) {
const toReturn = super.onAdd.apply(this, args);
if (this._map && this._map._loaded && !this._map.loaded()) {
this.mapState();
}
return toReturn;
}
};
}
```

I can put this fix in as a PR but I don't know enough about mapbox to know if theres another way to fix this or if there are any negative repercussions from adding the layer/source while the style or sources on the map are dirty.

Contributor guide

Open the contributing guide

Research direction

Start in src/directions.js around line 83 and compare its map.loaded() check with the linked mapbox-gl-js map.js behavior. Reproduce the case where _loaded is true but loaded() is false, then determine how onAdd should initialize directions without waiting for an event that will not fire. Done means the directions layer and source are added reliably in this state without regressions.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.