Skip to content

Commit

Permalink
Don't silently drop error events (#2852)
Browse files Browse the repository at this point in the history 
* Don't silently drop error events

fixes #2447

* Remove console.trace call

* Remove onError method in favor of Evented error fallback

* Restore "logs errors that happen during render" test

* Add error event docs

* Prevent test suite crashes on error
  • Loading branch information
@lucaswoj
lucaswoj authored Jul 20, 2016
1 parent 7ec9bbb commit 86349ac
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 58 deletions.
41 changes: 17 additions & 24 deletions js/ui/map.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,6 @@ var Map = module.exports = function(options) {
'_onSourceUpdate',
'_onWindowOnline',
'_onWindowResize',
'onError',
'_update',
'_render'
], this);
Expand Down Expand Up @@ -206,11 +205,11 @@ var Map = module.exports = function(options) {
if (options.style) this.setStyle(options.style);
if (options.attributionControl) this.addControl(new Attribution(options.attributionControl));

this.on('error', this.onError);
this.on('style.error', this.onError);
this.on('source.error', this.onError);
this.on('tile.error', this.onError);
this.on('layer.error', this.onError);
var fireError = this.fire.bind(this, 'error');
this.on('style.error', fireError);
this.on('source.error', fireError);
this.on('tile.error', fireError);
this.on('layer.error', fireError);
};

util.extend(Map.prototype, Evented);
Expand Down Expand Up @@ -1051,24 +1050,6 @@ util.extend(Map.prototype, /** @lends Map.prototype */{
this._container.classList.remove('mapboxgl-map');
},

/**
* Gets and sets an error handler for `style.error`, `source.error`, `layer.error`,
* and `tile.error` events.
*
* The default function logs errors with `console.error`.
*
* @example
* // Disable the default error handler
* map.off('error', map.onError);
* map.off('style.error', map.onError);
* map.off('source.error', map.onError);
* map.off('tile.error', map.onError);
* map.off('layer.error', map.onError);
*/
onError: function(e) {
console.error(e.error);
},

_rerender: function() {
if (this.style && !this._frameId) {
this._frameId = browser.frame(this._render);
Expand Down Expand Up @@ -1433,3 +1414,15 @@ function removeNode(node) {
* @instance
* @property {MapMouseEvent | MapTouchEvent} data
*/

/**
* Fired if any error occurs. This is GL JS's primary error reporting
* mechanism. We use an event instead of `throw` to better accommodate
* asyncronous operations. If no listeners are bound to the `error` event, the
* error will be printed to the console.
*
* @event error
* @memberof Map
* @instance
* @property {{error: {message: string}}} data
*/
9 changes: 8 additions & 1 deletion js/util/evented.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,14 @@ var Evented = {
* @returns {Object} `this`
*/
fire: function(type, data) {
if (!this.listens(type)) return this;
if (!this.listens(type)) {
// To ensure that no error events are dropped, print them to the
// console if they have no listeners.
if (util.endsWith(type, 'error')) {
console.error((data && data.error) || data || 'Empty error event');
}
return this;
}

data = util.extend({}, data);
util.extend(data, {type: type, target: this});
Expand Down
8 changes: 8 additions & 0 deletions test/js/style/style.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,10 @@ test('Style#removeSource', function(t) {
style.addSource('source-id', source);
style.removeSource('source-id');

// Bind a listener to prevent fallback Evented error reporting.
source.on('error', function() {});
source.on('tile.error', function() {});

source.fire('load');
source.fire('error');
source.fire('change');
Expand Down Expand Up @@ -689,6 +693,10 @@ test('Style#removeLayer', function(t) {
style.on('load', function() {
var layer = style._layers.background;
style.removeLayer('background');

// Bind a listener to prevent fallback Evented error reporting.
layer.on('error', function() {});

layer.fire('error', {mapbox: true});
t.end();
});
Expand Down
58 changes: 29 additions & 29 deletions test/js/ui/map.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,8 @@ test('Map', function(t) {
map.on('tile.error', checkEvent);
map.on('tile.remove', checkEvent);

map.off('style.error', map.onError);
map.off('source.error', map.onError);
map.off('tile.error', map.onError);
map.off('layer.error', map.onError);
// Suppress error messages
map.on('error', function() {});

t.plan(10);
map.setStyle(style); // Fires load
Expand All @@ -150,7 +148,6 @@ test('Map', function(t) {
style.fire('tile.add');
style.fire('tile.error');
style.fire('tile.remove');
style.fire('layer.error');
});

t.test('can be called more than once', function(t) {
Expand Down Expand Up @@ -750,7 +747,7 @@ test('Map', function(t) {
"sources": {
"mapbox://mapbox.satellite": {
"type": "raster",
"tiles": ["local://tiles/{z}-{x}-{y}.png"]
"tiles": ["http://example.com/{z}/{x}/{y}.png"]
}
},
"layers": [{
Expand All @@ -764,8 +761,8 @@ test('Map', function(t) {
}
});

// We're faking tiles
map.off('tile.error', map.onError);
// Suppress errors because we're not loading tiles from a real URL.
map.on('error', function() {});

map.on('style.load', function () {
map.setLayoutProperty('satellite', 'visibility', 'visible');
Expand Down Expand Up @@ -884,34 +881,32 @@ test('Map', function(t) {
t.end();
});

t.test('#onError', function (t) {
t.test('logs errors to console by default', function (t) {
var error = console.error;

console.error = function (e) {
console.error = error;
t.deepEqual(e.message, 'version: expected one of [8], 7 found');
t.test('error event', function (t) {
t.test('logs errors to console when it has NO listeners', function (t) {
sinon.stub(console, 'error', function(error) {
console.error.restore();
t.equal(error.message, 'version: expected one of [8], 7 found');
t.end();
};

createMap({
style: {
version: 7,
sources: {},
layers: []
}
});

createMap({ style: { version: 7, sources: {}, layers: [] } });
});

t.test('logs errors that happen during render', function (t) {
var error = console.error;
t.test('calls listeners', function (t) {
sinon.stub(console, 'error', function() {
console.error.restore();
t.fail();
});

console.error = function (e) {
console.error = error;
t.deepEqual(e.message, 'in render');
var map = createMap({ style: { version: 8, sources: {}, layers: [] } });
map.on('error', function(event) {
t.equal(event.error.message, 'version: expected one of [8], 7 found');
t.end();
};
});
map.setStyle({ version: 7, sources: {}, layers: [] });
});

t.test('logs errors that happen during render', function (t) {
var map = createMap({
style: {
version: 8,
Expand All @@ -924,6 +919,11 @@ test('Map', function(t) {
throw new Error('in render');
});

map.on('error', function (event) {
t.equal(event.error.message, 'in render');
t.end();
});

map._rerender = function () {};
map._render();
});
Expand Down
4 changes: 0 additions & 4 deletions test/suite_implementation.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,6 @@ module.exports = function(style, options, _callback) {

var gl = map.painter.gl;

map.on('error', function(event) {
callback(event.error);
});

map.once('load', function() {
applyOperations(map, options.operations, function() {
var w = options.width * browser.devicePixelRatio;
Expand Down

0 comments on commit 86349ac

Please sign in to comment.