Improve layer load #623
Improve layer load #623
Conversation
test/unit/api/layer.test.js
Outdated
| }; | ||
|
|
||
| const mapMock = { | ||
| isStyleLoaded: () => true, |
There was a problem hiding this comment.
jasmine.createSpy('isStyleLoaded').and.returnValue(true); 🙄
src/api/layer.js
Outdated
| map.on('load', () => { | ||
| this._onMapLoaded(map, beforeLayerID); | ||
| }); | ||
| map.once('data', (event) => { |
There was a problem hiding this comment.
I don't understand why does this work? Does it work? I thought that MGL required the style to be loaded before the addLayer method works
There was a problem hiding this comment.
MGL requires the style to be loaded if the data event type is sytle. About the data event, after reading the documentation I think it is the only change that needs to be listened, and only the first time (that is why I am using once).
https://www.mapbox.com/mapbox-gl-js/api/#map.event:data
Fired when any map data loads or changes
The examples in the editor work, but I agree it needs further testing.
test/unit/api/layer.test.js
Outdated
| }); | ||
|
|
||
| it('should call onMapLoaded when the map `load` event is triggered', () => { | ||
| describe('and dataType is style', () => { |
src/api/layer.js
Outdated
| this._isSourceLoaded = false; | ||
|
|
||
| map.on('sourcedata', this._onMapSourcedata); | ||
| map.on('load', this._onMapLoad.bind(this, map, beforeLayerID)); |
There was a problem hiding this comment.
I think
map.on('load', this._onMapLoad.bind(this));is more intuitive since we only bound the context but the parameters
src/api/layer.js
Outdated
| if (map.isStyleLoaded()) { | ||
| this._isSourceLoaded = false; | ||
|
|
||
| map.on('sourcedata', this._onMapSourcedata); |
There was a problem hiding this comment.
dont we need to bind this here?
src/api/layer.js
Outdated
| } | ||
|
|
||
| _onMapLoad(map, beforeLayerID) { | ||
| if (map.isStyleLoaded() || this._isSourceLoaded) { |
There was a problem hiding this comment.
I think this._isSourceLoaded is always undefined because this (pun intended) 
src/Layer.js
Outdated
| this._onMapLoaded(map, beforeLayerID); | ||
| } else { | ||
| } catch (error) { |
There was a problem hiding this comment.
since multiple errors could happen here I'd look for the specific mapbox error and act accordingly
There was a problem hiding this comment.
I agree with that, but I am not sure how to check it since error is a string and the message may vary also. This is how the error is triggered
_checkLoaded() {
if (!this._loaded) {
throw new Error('Style is not done loading');
}
}There was a problem hiding this comment.
catch(err) {
// We can control the situation when the error is "style not done loading"
if(err.message === 'Style is not done loading') {
this._onMapLoaded(map, beforeLayerID);
return;
}
// Otherwise a "real" error happened, forward it
throw error;
}
Related with #604
Related closed bugfix: bef7482
Current solution
We have two ways to handle this issue.
Use
try catchto handle mapbox error when style is not loaded. We listen tomap.on('load')to add the layer if the error has been triggered.Check
map.style._loaded.If this property is false, then we listen to
map.on('load')to add the layer.Both of them have been tested and they work, but we have chosen the first one because the second one depends on a private variable that may change.
Previous research:
We can not relay on
map.isStyleLoaded()to safely add a layer.There is an interesting Mapbox issue thread where this problem is being discussed. Highlights:
Another problem is that when trying to add a layer, mapbox-gl throws an error if the styles are not loaded. That is why we need to check somehow that the styles have been loaded. Since there is not a safe way to do this check, we would probably have to change our fork.