From d69d52e317b4c637a2b4a42522be3d16671e208a Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Wed, 10 Aug 2016 22:15:42 -0400 Subject: [PATCH 1/5] Allow multiple addSourceType calls with same name Since `addSourceType` adds the source type to a static map, it was yielding an error if two map instances tried to add the same source type. This changes `addSourceType(name, SourceType)` to simply do nothing if there is already a source type named `name`. --- js/style/style.js | 2 +- test/js/style/style.test.js | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/js/style/style.js b/js/style/style.js index 83a1ee638f0..38cbc75d54d 100644 --- a/js/style/style.js +++ b/js/style/style.js @@ -671,7 +671,7 @@ Style.prototype = util.inherit(Evented, { addSourceType: function (name, SourceType, callback) { if (Source.getType(name)) { - return callback(new Error('A source type called "' + name + '" already exists.')); + return callback(null, null); } Source.setType(name, SourceType); diff --git a/test/js/style/style.test.js b/test/js/style/style.test.js index d36dfc47e20..6e20250640d 100644 --- a/test/js/style/style.test.js +++ b/test/js/style/style.test.js @@ -1222,10 +1222,12 @@ test('Style#addSourceType', function (t) { style.addSourceType('bar', SourceType, function (err) { t.error(err); }); }); - t.test('refuses to add new type over existing name', function (t) { + t.test('does not overwrite existing source type, but returns gracefully', function (t) { var style = new Style(createStyleJSON()); + var existing = _types['existing']; style.addSourceType('existing', function () {}, function (err) { - t.ok(err); + t.error(err); + t.equal(_types['existing'], existing); t.end(); }); }); From 888a8defccf135e94b289a57876749f6c2dd51fa Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Mon, 15 Aug 2016 15:13:41 -0400 Subject: [PATCH 2/5] Register existing custom sources before style load --- js/source/source.js | 12 ++++++++++++ js/style/style.js | 32 +++++++++++++++++++++++++++----- test/js/style/style.test.js | 33 +++++++++++++++++++++++++++++++-- 3 files changed, 70 insertions(+), 7 deletions(-) diff --git a/js/source/source.js b/js/source/source.js index 7d98b4d7fad..00f65e76d5b 100644 --- a/js/source/source.js +++ b/js/source/source.js @@ -10,6 +10,8 @@ var sourceTypes = { 'image': require('../source/image_source') }; +var coreTypes = ['vector', 'raster', 'geojson', 'video', 'image']; + /* * Creates a tiled data source instance given an options object. * @@ -38,6 +40,16 @@ exports.setType = function (name, type) { sourceTypes[name] = type; }; +/** + * Returns the names of any registered non-core source types. + * @private + */ +exports.getCustomTypeNames = function () { + return Object.keys(sourceTypes).filter(function (type) { + return coreTypes.indexOf(type) < 0; + }); +}; + /** * The `Source` interface must be implemented by each source type, including "core" types (`vector`, `raster`, `video`, etc.) and all custom, third-party types. * diff --git a/js/style/style.js b/js/style/style.js index 38cbc75d54d..69b543e0c6d 100644 --- a/js/style/style.js +++ b/js/style/style.js @@ -1,5 +1,6 @@ 'use strict'; +var assert = require('assert'); var Evented = require('../util/evented'); var StyleLayer = require('./style_layer'); var ImageSprite = require('./image_sprite'); @@ -70,11 +71,32 @@ function Style(stylesheet, animationLoop, workerCount) { this.fire('load'); }.bind(this); - if (typeof stylesheet === 'string') { - ajax.getJSON(normalizeURL(stylesheet), stylesheetLoaded); - } else { - browser.frame(stylesheetLoaded.bind(this, null, stylesheet)); - } + // register any existing custom source types with the workers + util.asyncAll(Source.getCustomTypeNames(), function (type, done) { + var CustomSource = Source.getType(type); + assert(CustomSource); + + if (CustomSource.workerSourceURL) { + this.dispatcher.broadcast('load worker source', { + name: type, + url: CustomSource.workerSourceURL + }, done); + } else { + return done(); + } + }.bind(this), function (err) { + if (err) { + this.fire('error', {error: err}); + return; + } + + if (typeof stylesheet === 'string') { + ajax.getJSON(normalizeURL(stylesheet), stylesheetLoaded); + } else { + browser.frame(stylesheetLoaded.bind(this, null, stylesheet)); + } + }.bind(this)); + this.on('source.load', function(event) { var source = event.source; diff --git a/test/js/style/style.test.js b/test/js/style/style.test.js index 6e20250640d..d589bde5d76 100644 --- a/test/js/style/style.test.js +++ b/test/js/style/style.test.js @@ -131,6 +131,34 @@ test('Style', function(t) { }); }); + t.test('registers existing custom sources', function (t) { + function SourceType () {} + SourceType.workerSourceURL = 'worker-source.js'; + function Dispatcher () {} + Dispatcher.prototype = { + broadcast: function (type, params, callback) { + if (type === 'load worker source') { + t.equal(params.name, 'my-source-type'); + t.equal(params.url, 'worker-source.js'); + setTimeout(callback, 0); + } + } + }; + + t.plan(2); + + var Style = proxyquire('../../../js/style/style', { + '../source/source': { + getType: function () { return SourceType; }, + setType: function () {}, + getCustomTypeNames: function () { return ['my-source-type']; } + }, + '../util/dispatcher': Dispatcher + }); + + new Style(createStyleJSON()); + }); + t.end(); }); @@ -1184,11 +1212,12 @@ test('Style#addSourceType', function (t) { var Style = proxyquire('../../../js/style/style', { '../source/source': { getType: function (name) { return _types[name]; }, - setType: function (name, create) { _types[name] = create; } + setType: function (name, create) { _types[name] = create; }, + getTypeNames: function () { return []; } } }); - t.test('adds factory function', function (t) { + t.test('adds source type', function (t) { var style = new Style(createStyleJSON()); var SourceType = function () {}; From cf7e5df999bf3bc21c57655930f89e75065e7c47 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Mon, 15 Aug 2016 16:54:32 -0400 Subject: [PATCH 3/5] Move Map#addSourceType to Source --- js/source/source.js | 35 ++++++++++--- js/style/style.js | 62 +++++++++++------------ js/ui/map.js | 14 +----- test/js/source/source.test.js | 36 +++++++++++++ test/js/style/style.test.js | 95 +++++++++++------------------------ 5 files changed, 125 insertions(+), 117 deletions(-) create mode 100644 test/js/source/source.test.js diff --git a/js/source/source.js b/js/source/source.js index 00f65e76d5b..fd65f53f4eb 100644 --- a/js/source/source.js +++ b/js/source/source.js @@ -1,5 +1,6 @@ 'use strict'; +var Evented = require('../util/evented'); var util = require('../util/util'); var sourceTypes = { @@ -12,6 +13,8 @@ var sourceTypes = { var coreTypes = ['vector', 'raster', 'geojson', 'video', 'image']; +var Source = module.exports = util.extend({}, Evented); + /* * Creates a tiled data source instance given an options object. * @@ -21,7 +24,7 @@ var coreTypes = ['vector', 'raster', 'geojson', 'video', 'image']; * @param {Dispatcher} dispatcher * @returns {Source} */ -exports.create = function(id, source, dispatcher) { +Source.create = function(id, source, dispatcher) { source = new sourceTypes[source.type](id, source, dispatcher); if (source.id !== id) { @@ -32,11 +35,11 @@ exports.create = function(id, source, dispatcher) { return source; }; -exports.getType = function (name) { +Source.getType = function (name) { return sourceTypes[name]; }; -exports.setType = function (name, type) { +Source.setType = function (name, type) { sourceTypes[name] = type; }; @@ -44,12 +47,32 @@ exports.setType = function (name, type) { * Returns the names of any registered non-core source types. * @private */ -exports.getCustomTypeNames = function () { +Source.getCustomTypeNames = function () { return Object.keys(sourceTypes).filter(function (type) { return coreTypes.indexOf(type) < 0; }); }; +/** + * Adds a [custom source type](#Custom Sources), making it available for use with + * {@link Map#addSource}. + * @private + * @param {string} name The name of the source type; source definition objects use this name in the `{type: ...}` field. + * @param {Function} SourceType A {@link Source} constructor. + */ +Source.addType = function (name, SourceType) { + if (Source.getType(name)) { + throw new Error('A source type named ' + name + ' already exists.'); + } + + Source.setType(name, SourceType); + + // an internal event, used to notify Style instances that there is a new + // custom source type. + Source.fire('_add', { name: name }); +}; + + /** * The `Source` interface must be implemented by each source type, including "core" types (`vector`, `raster`, `video`, etc.) and all custom, third-party types. * @@ -58,7 +81,7 @@ exports.getCustomTypeNames = function () { * * @param {string} id The id for the source. Must not be used by any existing source. * @param {Object} options Source options, specific to the source type (except for `options.type`, which is always required). - * @param {string} options.type The source type, matching the value of `name` used in {@link Style#addSourceType}. + * @param {string} options.type The source type, matching the value of `name` used in {@link Source.addType}. * @param {Dispatcher} dispatcher A {@link Dispatcher} instance, which can be used to send messages to the workers. * * @fires load to indicate source data has been loaded, so that it's okay to call `loadTile` @@ -128,7 +151,7 @@ exports.getCustomTypeNames = function () { * implementation may also be targeted by the {@link Source} via * `dispatcher.send('source-type.methodname', params, callback)`. * - * @see {@link Map#addSourceType} + * @see {@link Source.addType} * @private * * @class WorkerSource diff --git a/js/style/style.js b/js/style/style.js index 69b543e0c6d..3e950d14f78 100644 --- a/js/style/style.js +++ b/js/style/style.js @@ -38,11 +38,15 @@ function Style(stylesheet, animationLoop, workerCount) { '_forwardSourceEvent', '_forwardTileEvent', '_forwardLayerEvent', - '_redoPlacement' + '_redoPlacement', + '_handleAddSourceType', + '_registerCustomSource' ], this); this._resetUpdates(); + Source.on('_add', this._handleAddSourceType); + var stylesheetLoaded = function(err, stylesheet) { if (err) { this.fire('error', {error: err}); @@ -72,19 +76,7 @@ function Style(stylesheet, animationLoop, workerCount) { }.bind(this); // register any existing custom source types with the workers - util.asyncAll(Source.getCustomTypeNames(), function (type, done) { - var CustomSource = Source.getType(type); - assert(CustomSource); - - if (CustomSource.workerSourceURL) { - this.dispatcher.broadcast('load worker source', { - name: type, - url: CustomSource.workerSourceURL - }, done); - } else { - return done(); - } - }.bind(this), function (err) { + util.asyncAll(Source.getCustomTypeNames(), this._registerCustomSource, function (err) { if (err) { this.fire('error', {error: err}); return; @@ -97,7 +89,6 @@ function Style(stylesheet, animationLoop, workerCount) { } }.bind(this)); - this.on('source.load', function(event) { var source = event.source; if (source && source.vectorLayerIds) { @@ -691,23 +682,6 @@ Style.prototype = util.inherit(Evented, { return source ? QueryFeatures.source(source, params) : []; }, - addSourceType: function (name, SourceType, callback) { - if (Source.getType(name)) { - return callback(null, null); - } - - Source.setType(name, SourceType); - - if (!SourceType.workerSourceURL) { - return callback(null, null); - } - - this.dispatcher.broadcast('load worker source', { - name: name, - url: SourceType.workerSourceURL - }, callback); - }, - _handleErrors: function(validate, key, value, throws, props) { var action = throws ? validateStyle.throwErrors : validateStyle.emitErrors; var result = validate.call(validateStyle, util.extend({ @@ -719,8 +693,32 @@ Style.prototype = util.inherit(Evented, { return action.call(validateStyle, this, result); }, + _handleAddSourceType: function (event) { + this._registerCustomSource(event.name, function (err) { + if (err) { + this.fire('error', {error: err}); + return; + } + this.fire('source-type.add', event); + }.bind(this)); + }, + + _registerCustomSource: function (name, callback) { + var SourceType = Source.getType(name); + assert(SourceType); + if (SourceType.workerSourceURL) { + this.dispatcher.broadcast('load worker source', { + name: name, + url: SourceType.workerSourceURL + }, callback); + } else { + callback(); + } + }, + _remove: function() { this.dispatcher.remove(); + Source.off('_add', this._handleAddSourceType); }, _reloadSource: function(id) { diff --git a/js/ui/map.js b/js/ui/map.js index a16b8c1958a..5bcc5a15a02 100755 --- a/js/ui/map.js +++ b/js/ui/map.js @@ -685,7 +685,7 @@ util.extend(Map.prototype, /** @lends Map.prototype */{ * @param {string} id The ID of the source to add. Must not conflict with existing sources. * @param {Object} source The source object, conforming to the * Mapbox Style Specification's [source definition](https://www.mapbox.com/mapbox-gl-style-spec/#sources). - * @param {string} source.type The source type, which must be either one of the core Mapbox GL source types defined in the style specification or a custom type that has been added to the map with {@link Map#addSourceType}. + * @param {string} source.type The source type, which must be one of the core Mapbox GL source types defined in the style specification. * @fires source.add * @returns {Map} `this` */ @@ -695,18 +695,6 @@ util.extend(Map.prototype, /** @lends Map.prototype */{ return this; }, - /** - * Adds a [custom source type](#Custom Sources), making it available for use with - * {@link Map#addSource}. - * @private - * @param {string} name The name of the source type; source definition objects use this name in the `{type: ...}` field. - * @param {Function} SourceType A {@link Source} constructor. - * @param {Function} callback Called when the source type is ready or with an error argument if there is an error. - */ - addSourceType: function (name, SourceType, callback) { - return this.style.addSourceType(name, SourceType, callback); - }, - /** * Removes a source from the map's style. * diff --git a/test/js/source/source.test.js b/test/js/source/source.test.js new file mode 100644 index 00000000000..8a0a809fbc0 --- /dev/null +++ b/test/js/source/source.test.js @@ -0,0 +1,36 @@ +'use strict'; + +var test = require('tap').test; +var Source = require('../../../js/source/source'); + +test('Source', function (t) { + t.test('#getCustomTypeNames', function (t) { + t.same(Source.getCustomTypeNames(), []); + Source.addType('source.test.type-1', function () {}); + t.same(Source.getCustomTypeNames(), ['source.test.type-1']); + t.end(); + }); + + t.test('#addType', function (t) { + function SourceType () {} + Source.on('_add', onAdd); + + t.plan(2); + Source.addType('source.test.type-2', SourceType); + t.equal(Source.getType('source.test.type-2'), SourceType); + function onAdd (event) { + t.equal(event.name, 'source.test.type-2'); + Source.off('_add', onAdd); + } + }); + + t.test('#addType throws for duplicate source type', function (t) { + Source.addType('source.test.type-3', function () {}); + t.throws(function () { + Source.addType('source.test.type-3', function () {}); + }); + t.end(); + }); + + t.end(); +}); diff --git a/test/js/style/style.test.js b/test/js/style/style.test.js index d589bde5d76..44a9890b1e5 100644 --- a/test/js/style/style.test.js +++ b/test/js/style/style.test.js @@ -131,27 +131,47 @@ test('Style', function(t) { }); }); - t.test('registers existing custom sources', function (t) { - function SourceType () {} - SourceType.workerSourceURL = 'worker-source.js'; + t.test('registers WorkerSource for custom sources', function (t) { + function MySourceType () {} + MySourceType.workerSourceURL = 'my-worker-source.js'; + function LaterSourceType () {} + LaterSourceType.workerSourceURL = 'later-worker-source.js'; + function WorkerlessSourceType () {} + var _types = { 'my-source-type': MySourceType, 'workerless': WorkerlessSourceType }; + + var expected = [ + { name: 'my-source-type', url: 'my-worker-source.js' }, + { name: 'later-source-type', url: 'later-worker-source.js' } + ]; + + t.plan(2 * expected.length); + function Dispatcher () {} Dispatcher.prototype = { broadcast: function (type, params, callback) { if (type === 'load worker source') { - t.equal(params.name, 'my-source-type'); - t.equal(params.url, 'worker-source.js'); + var exp = expected.shift(); + t.equal(params.name, exp.name); + t.equal(params.url, exp.url); setTimeout(callback, 0); } } }; - t.plan(2); - var Style = proxyquire('../../../js/style/style', { '../source/source': { - getType: function () { return SourceType; }, + getType: function (name) { return _types[name]; }, setType: function () {}, - getCustomTypeNames: function () { return ['my-source-type']; } + getCustomTypeNames: function () { return Object.keys(_types); }, + on: function (type, handler) { + if (type === '_add') { + setTimeout(function () { + _types['later-source-type'] = LaterSourceType; + handler({ name: 'later-source-type' }); + }); + } + }, + off: function () {} }, '../util/dispatcher': Dispatcher }); @@ -1207,63 +1227,6 @@ test('Style#query*Features', function(t) { t.end(); }); -test('Style#addSourceType', function (t) { - var _types = { 'existing': function () {} }; - var Style = proxyquire('../../../js/style/style', { - '../source/source': { - getType: function (name) { return _types[name]; }, - setType: function (name, create) { _types[name] = create; }, - getTypeNames: function () { return []; } - } - }); - - t.test('adds source type', function (t) { - var style = new Style(createStyleJSON()); - var SourceType = function () {}; - - // expect no call to load worker source - style.dispatcher.broadcast = function (type) { - if (type === 'load worker source') { - t.fail(); - } - }; - - style.addSourceType('foo', SourceType, function () { - t.equal(_types['foo'], SourceType); - t.end(); - }); - }); - - t.test('triggers workers to load worker source code', function (t) { - var style = new Style(createStyleJSON()); - var SourceType = function () {}; - SourceType.workerSourceURL = 'worker-source.js'; - - style.dispatcher.broadcast = function (type, params) { - if (type === 'load worker source') { - t.equal(_types['bar'], SourceType); - t.equal(params.name, 'bar'); - t.equal(params.url, 'worker-source.js'); - t.end(); - } - }; - - style.addSourceType('bar', SourceType, function (err) { t.error(err); }); - }); - - t.test('does not overwrite existing source type, but returns gracefully', function (t) { - var style = new Style(createStyleJSON()); - var existing = _types['existing']; - style.addSourceType('existing', function () {}, function (err) { - t.error(err); - t.equal(_types['existing'], existing); - t.end(); - }); - }); - - t.end(); -}); - test('Style creates correct number of workers', function(t) { var style = new Style(createStyleJSON(), null, 3); t.equal(style.dispatcher.actors.length, 3); From 9438bc40fbe5b4f5c0079b0fc0ef84fc82e4e2d6 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Mon, 15 Aug 2016 17:46:48 -0400 Subject: [PATCH 4/5] Accept sourceTypes option in Map constructor --- js/ui/map.js | 6 ++++++ test/js/ui/map.test.js | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/js/ui/map.js b/js/ui/map.js index 5bcc5a15a02..3b4fce0df79 100755 --- a/js/ui/map.js +++ b/js/ui/map.js @@ -7,6 +7,7 @@ var window = require('../util/browser').window; var Evented = require('../util/evented'); var DOM = require('../util/dom'); +var Source = require('../source/source'); var Style = require('../style/style'); var AnimationLoop = require('../style/animation_loop'); var Painter = require('../render/painter'); @@ -202,6 +203,11 @@ var Map = module.exports = function(options) { this.resize(); if (options.classes) this.setClasses(options.classes); + if (options.sourceTypes) { + for (var sourceTypeName in options.sourceTypes) { + Source.addType(sourceTypeName, options.sourceTypes[sourceTypeName]); + } + } if (options.style) this.setStyle(options.style); if (options.attributionControl) this.addControl(new Attribution(options.attributionControl)); diff --git a/test/js/ui/map.test.js b/test/js/ui/map.test.js index cee6216579d..b4385993038 100755 --- a/test/js/ui/map.test.js +++ b/test/js/ui/map.test.js @@ -8,6 +8,7 @@ var Style = require('../../../js/style/style'); var LngLat = require('../../../js/geo/lng_lat'); var browser = require('../../../js/util/browser'); var sinon = require('sinon'); +var proxyquire = require('proxyquire'); var fixed = require('../../testutil/fixed'); var fixedNum = fixed.Num; @@ -94,6 +95,45 @@ test('Map', function(t) { t.end(); }); + t.test('creates source types before style is set', function (t) { + var sourceTypesAdded = []; + var Map = proxyquire('../../../js/ui/map', { + '../source/source': { + addType: function (name, SourceType) { + t.ok(typeof SourceType === 'function'); + sourceTypesAdded.push(name); + } + }, + '../style/style': function () { + t.same(sourceTypesAdded, ['tic', 'tac']); + t.end(); + this.on = function () { return this; }; + } + }); + + new Map({ + container: { + offsetWidth: 200, + offsetHeight: 200, + classList: { + add: function() {}, + remove: function() {} + } + }, + interactive: false, + attributionControl: false, + sourceTypes: { + 'tic': function () {}, + 'tac': function () {} + }, + style: { + "version": 8, + "sources": {}, + "layers": [] + } + }); + }); + t.test('emits load event after a style is set', function(t) { var map = createMap(); From 92dac0683717ee9810981dc4fb4dde937e5296ab Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Mon, 15 Aug 2016 18:23:20 -0400 Subject: [PATCH 5/5] Export mapboxgl.Source --- js/mapbox-gl.js | 1 + 1 file changed, 1 insertion(+) diff --git a/js/mapbox-gl.js b/js/mapbox-gl.js index d1533d84343..efebf4e3ce0 100644 --- a/js/mapbox-gl.js +++ b/js/mapbox-gl.js @@ -14,6 +14,7 @@ mapboxgl.Popup = require('./ui/popup'); mapboxgl.Marker = require('./ui/marker'); mapboxgl.Style = require('./style/style'); +mapboxgl.Source = require('./source/source'); mapboxgl.LngLat = require('./geo/lng_lat'); mapboxgl.LngLatBounds = require('./geo/lng_lat_bounds');