Merge pull request mapbox#2982 from mapbox/fix-add-source-type

Revise addSourceType to be independent of Map instance
developmentseed · Aug 20, 2016 · 0699cf3 · 0699cf3
2 parents 3ff8138 + 5dae4c7
commit 0699cf3
Show file tree

Hide file tree

Showing 8 changed files with 216 additions and 96 deletions.
diff --git 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');

diff --git 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 = {
@@ -10,6 +11,10 @@ var sourceTypes = {
     'image': require('../source/image_source')
 };
 
+var coreTypes = ['vector', 'raster', 'geojson', 'video', 'image'];
+
+var Source = module.exports = util.extend({}, Evented);
+
 /*
  * Creates a tiled data source instance given an options object.
  *
@@ -19,7 +24,7 @@ var sourceTypes = {
  * @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) {
@@ -30,14 +35,44 @@ 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;
 };
 
+/**
+ * Returns the names of any registered non-core source types.
+ * @private
+ */
+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.
  *
@@ -46,7 +81,7 @@ exports.setType = function (name, type) {
  *
  * @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`
@@ -116,7 +151,7 @@ exports.setType = function (name, type) {
  * 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/source/worker.js b/js/source/worker.js
@@ -28,7 +28,7 @@ function Worker(self) {
 
     this.self.registerWorkerSource = function (name, WorkerSource) {
         if (this.workerSources[name]) {
-            throw new Error('Worker source with name "' + name + '" already registered.');
+            util.warnOnce('Worker source named "' + name + '" already registered.');
         }
         this.workerSources[name] = new WorkerSource(this.actor, styleLayers);
     }.bind(this);

diff --git 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');
@@ -37,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});
@@ -70,11 +75,19 @@ 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(), this._registerCustomSource, 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;
@@ -669,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(new Error('A source type called "' + name + '" already exists.'));
-        }
-
-        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({
@@ -697,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
@@ -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.customSourceTypes) {
+        for (var sourceTypeName in options.customSourceTypes) {
+            Source.addType(sourceTypeName, options.customSourceTypes[sourceTypeName]);
+        }
+    }
     if (options.style) this.setStyle(options.style);
     if (options.attributionControl) this.addControl(new Attribution(options.attributionControl));
 
@@ -685,7 +691,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 +701,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
@@ -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();
+});