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

[rhagigi]

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.

[rhagigi]

@tmcw ^ Let me know if you think it makes sense to push the above as a PR (changing this.map.loaded() to this.map._loaded -- or some other way to fix this issue)

[vanhumbeecka]

@rhagigi thanks for posting this fix. Just what I needed, and works perfectly.

I believe the real reason behind this mess is the mapbox API could have picked a better name for loaded() function, since you would expect it to return true when the load event triggered, but this sadly not the case - leading to a lot of confustion and bugs (such as this) in different codebases.

Maybe there is still 1 issue with this code, but it's a minor one: When calling super.onAdd.apply(this, args), the plugin while mostly return false on the loaded() function, resulting in a new event-listener on the load event. If you repeatedly then remove and add this plugin to the map, the event-listeners will pile up on this code, but will otherwise do no harm.