Revise addSourceType to be independent of Map instance #2982

anandthakker · 2016-08-11T02:25:21Z

The problem: addSourceType does not quite make sense as a method on the Map (or Style) instance, because: a) Sources, being types, are really 'global'; b) practically speaking, there are problems having this as an instance method, because that approach fails to coordinate setting up the WorkerSources for other map instances.

Changes:

Replace Map#addSourceType and Style#addSourceType with the static Source.addType -- exported as mapboxgl.addSourceType. Leveraging the global worker pool, this method can now directly take care of registering the WorkerSource for the added source type if necessary.

Checklist

briefly describe the changes in this PR
write tests for all new functionality
document any changes or additions to public APIs (skipping this because we are keeping the API private until it's vetted)
post scores for all benchmarks on your branch and master
do a quick sanity check on the debug page
get a PR review 👍 / 👎

anandthakker · 2016-08-15T19:25:44Z

I'm going to revise this a bit so that it can address #3004

anandthakker · 2016-08-15T22:03:44Z

@lucaswoj ^ revised this to be a more comprehensive solution. Ready for your 👀

anandthakker · 2016-08-15T22:04:18Z

(also edited the top of the ticket)

anandthakker · 2016-08-15T22:45:40Z

Here is a working example of the API that this change affords: https://github.com/developmentseed/mapbox-gl-topojson/blob/gh-pages/site.js

Revise addSourceType to be independent of Map instance

mourner · 2016-08-17T19:19:53Z

js/ui/map.js

@@ -202,6 +203,11 @@ var Map = module.exports = function(options) {
    this.resize();

    if (options.classes) this.setClasses(options.classes);
+    if (options.sourceTypes) {


Should we name it customSourceTypes to reflect that we supply those additionally to the core ones?

Makes sense to me, but I also don't have a strong opinion on this.

mourner · 2016-08-17T19:20:03Z

This generally looks good to me!

Revise addSourceType to be independent of Map instance

anandthakker · 2016-08-19T14:30:11Z

@mourner @lucaswoj added one more small change in the last commit. I believe this is ready to merge.

Revise addSourceType to be independent of Map instance

lucaswoj · 2016-08-22T20:41:48Z

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');


Can we expose mapboxgl.addSourceType instead of the whole Source module? There are lots of bits & pieces in Source that we don't want to be public.

anandthakker · 2016-08-24T04:04:57Z

Rebased, and updated to leverage the global worker pool.

Two key notes:

Note the fix in 8a7807f -- this isn't really the topic of this PR (it's actually a followup to Add a minimal global worker pool #2952 ), but the issue got in my way while I was here so I figured I'd include the fix.
~~There is an outstanding TODO here~~ See this note about workers being created (and not destroyed) when a custom worker source is added. I think this is "good enough" for now, because (a) it is only relevant when a custom source with WorkerSource is being used and all of the map instances on the page are being remove()ed; (b) I believe this would require acrobatics like making Dispatcher-creation asynchronous and giving WorkerPool knowledge of custom sources and (c) I think Refactor "Worker", "Dispatcher", "Actor" into a general purpose "PooledWorker" utility #3034 will afford a much cleaner way to handle this anyway, so why introduce ugly code that's soon to be obsolete?

anandthakker · 2016-08-31T11:42:21Z

@lucaswoj this should be good to go. If you have reservations about this PR or it needs more discussion, it might be worth pulling out 8a7807f into a separate one to merge sooner

Revise addSourceType to be independent of Map instance

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`.

anandthakker · 2016-09-13T22:27:49Z

Rebased

lucaswoj · 2016-09-14T23:12:03Z

I'm coming to see the wisdom behind GL Native's custom layer architecture. Instead of defining custom source types, we should accept custom source objects which conform to a given interface specification.

map.addCustomSource('foo', new WhizzBangSource({...}))

The benefits of this approach include

no need to worry about interactions between the style spec validator and custom sources
the ability to pass functions to custom sources (not allowed by the style spec)
smaller API surface / less boilerplate required to create a custom source
compatibility with the planned custom layer interface Support custom layer types powered by WebGL #281
compatibility with GL Native's custom layer interface

One downside is that we'll need to find a way to register worker-side logic without the existence of source "types". Does this seem doable @anandthakker? Would it be more doable if we implemented #3034?

Overall, does implementing this architecture sound doable within the scope of this PR?

anandthakker · 2016-09-15T18:36:31Z

@lucaswoj This is interesting! I actually don't think it would be too difficult to deal with registering worker-side code; we'd just need an API like mapboxgl.registerWorkerSource( url ), where the URL would be to a script pretty similar to that which is at present provided by CustomSourceType.workerSourceURL

However, one drawback to this no-types approach is that it basically seals the deal against specifying custom sources (and their dependent style layers) declaratively with style json like new Map(style: url). Personally, I think it's nice that the source type approach could enable this (after appropriately relaxing style validation).

lucaswoj · 2016-09-15T19:09:40Z

However, one drawback to this no-types approach is that it basically seals the deal against specifying custom sources (and their dependent style layers) declaratively with style json like new Map(style: url). Personally, I think it's nice that the source type approach could enable this (after appropriately relaxing style validation).

It's a tradeoff for sure. We are giving up some ergonomics in exchange for simplicity, uniformity with GL Native, and the preservation of a tight validator. I prefer to err on the side of simplicity and see how things shake out in practice. Down the road we may revisit the "register source type" design or a design that allows embedding Source-interfaced objects in calls to Map#setStyle.

anandthakker · 2016-09-15T19:28:23Z

It's a tradeoff for sure. We are giving up some ergonomics in exchange for a simplicity and the preservation of a tight validator. I prefer to err on the side of API simplicity (and uniformity with GL Native) and see how things shake out.

Gotcha.

I think the change from addSourceType-based to addCustomSource-based API should be reasonably manageable, but definitely makes more sense to do after the PooledWorker refactor that you're working on @lucaswoj.

Revise addSourceType to be independent of Map instance

lucaswoj · 2016-10-07T18:59:06Z

We are going to go a different direction for this functionality: #3004

anandthakker mentioned this pull request Aug 15, 2016

Accept custom source types before or during Map creation #3004

Closed

anandthakker added the do not merge label Aug 15, 2016

anandthakker force-pushed the fix-add-source-type branch from a5ccc4f to 9438bc4 Compare August 15, 2016 21:47

anandthakker changed the title ~~Allow multiple addSourceType calls with same name~~ Revise addSourceType to be independent of Map instance Aug 15, 2016

anandthakker removed the do not merge label Aug 15, 2016

anandthakker assigned lucaswoj Aug 15, 2016

anandthakker pushed a commit to developmentseed/mapbox-gl-topojson that referenced this pull request Aug 15, 2016

Update to match mapbox/mapbox-gl-js#2982

9ffbc1a

anandthakker pushed a commit to developmentseed/mapbox-gl-js that referenced this pull request Aug 16, 2016

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

363de05

Revise addSourceType to be independent of Map instance

anandthakker pushed a commit to developmentseed/mapbox-gl-js that referenced this pull request Aug 16, 2016

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

969185f

Revise addSourceType to be independent of Map instance

anandthakker pushed a commit to developmentseed/mapbox-gl-js that referenced this pull request Aug 16, 2016

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

6bd595a

Revise addSourceType to be independent of Map instance

anandthakker pushed a commit to developmentseed/mapbox-gl-js that referenced this pull request Aug 16, 2016

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

38b4581

Revise addSourceType to be independent of Map instance

mourner reviewed Aug 17, 2016
View reviewed changes

anandthakker pushed a commit to developmentseed/mapbox-gl-js that referenced this pull request Aug 17, 2016

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

3ac59a9

Revise addSourceType to be independent of Map instance

anandthakker pushed a commit to developmentseed/mapbox-gl-js that referenced this pull request Aug 18, 2016

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

d042521

Revise addSourceType to be independent of Map instance

anandthakker pushed a commit to developmentseed/mapbox-gl-js that referenced this pull request Aug 18, 2016

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

4bd9cc1

Revise addSourceType to be independent of Map instance

anandthakker pushed a commit to developmentseed/mapbox-gl-js that referenced this pull request Aug 19, 2016

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

ef8b3d1

Revise addSourceType to be independent of Map instance

anandthakker pushed a commit to developmentseed/mapbox-gl-js that referenced this pull request Aug 19, 2016

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

8810262

Revise addSourceType to be independent of Map instance

anandthakker pushed a commit to developmentseed/mapbox-gl-js that referenced this pull request Aug 19, 2016

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

44598a1

Revise addSourceType to be independent of Map instance

anandthakker pushed a commit to developmentseed/mapbox-gl-js that referenced this pull request Aug 20, 2016

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

0699cf3

Revise addSourceType to be independent of Map instance

anandthakker mentioned this pull request Aug 21, 2016

Add a minimal global worker pool #2952

Merged

8 tasks

lucaswoj reviewed Aug 22, 2016
View reviewed changes

anandthakker force-pushed the fix-add-source-type branch from 5dae4c7 to 5889145 Compare August 24, 2016 03:54

anandthakker force-pushed the fix-add-source-type branch from 5889145 to b632c7d Compare August 24, 2016 04:02

anandthakker mentioned this pull request Sep 1, 2016

Fix bug causing garbled text rendering when multiple maps are rendered on the page #3086

Closed

anandthakker pushed a commit to developmentseed/mapbox-gl-js that referenced this pull request Sep 2, 2016

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

cff4ee8

Revise addSourceType to be independent of Map instance

Anand Thakker added 11 commits September 13, 2016 17:47

Register existing custom sources before style load

e370c62

Move Map#addSourceType to Source

a6ce016

Export mapboxgl.addSourceType

229a1a4

Warn instead of error on duplicate worker source

bc7ff35

Move worker source registration to Source.addType

df0d221

Add temporary TODO

28c7f85

Update tests

ee2059b

Drop unused Evented

74b4e8a

Drop TODO, add note about worker lifecycle

36eb7cf

Remove old code from Style

9d5a87e

anandthakker force-pushed the fix-add-source-type branch from 98b7e28 to 9d5a87e Compare September 13, 2016 21:56

lucaswoj mentioned this pull request Sep 14, 2016

Refactor source interface to better accommodate non-tiled sources #3186

Open

Merge remote-tracking branch 'origin/master' into fix-add-source-type

b22d2ba

anandthakker pushed a commit to developmentseed/mapbox-gl-js that referenced this pull request Sep 27, 2016

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

3933072

Revise addSourceType to be independent of Map instance

lucaswoj mentioned this pull request Sep 27, 2016

Bucket enhancements to allow property-only update #2812

Closed

lucaswoj added the under construction label Oct 5, 2016

lucaswoj closed this Oct 7, 2016

jfirebaugh deleted the fix-add-source-type branch February 3, 2017 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revise addSourceType to be independent of Map instance #2982

Revise addSourceType to be independent of Map instance #2982

anandthakker commented Aug 11, 2016 •

edited

Loading

anandthakker commented Aug 15, 2016

anandthakker commented Aug 15, 2016

anandthakker commented Aug 15, 2016

anandthakker commented Aug 15, 2016

mourner Aug 17, 2016

anandthakker Aug 17, 2016

mourner commented Aug 17, 2016

anandthakker commented Aug 19, 2016

lucaswoj Aug 22, 2016

anandthakker commented Aug 24, 2016 •

edited

Loading

anandthakker commented Aug 31, 2016

anandthakker commented Sep 13, 2016

lucaswoj commented Sep 14, 2016 •

edited

Loading

anandthakker commented Sep 15, 2016

lucaswoj commented Sep 15, 2016 •

edited

Loading

anandthakker commented Sep 15, 2016

lucaswoj commented Oct 7, 2016

Revise addSourceType to be independent of Map instance #2982

Revise addSourceType to be independent of Map instance #2982

Conversation

anandthakker commented Aug 11, 2016 • edited Loading

Checklist

anandthakker commented Aug 15, 2016

anandthakker commented Aug 15, 2016

anandthakker commented Aug 15, 2016

anandthakker commented Aug 15, 2016

mourner Aug 17, 2016

Choose a reason for hiding this comment

anandthakker Aug 17, 2016

Choose a reason for hiding this comment

mourner commented Aug 17, 2016

anandthakker commented Aug 19, 2016

lucaswoj Aug 22, 2016

Choose a reason for hiding this comment

anandthakker commented Aug 24, 2016 • edited Loading

anandthakker commented Aug 31, 2016

anandthakker commented Sep 13, 2016

lucaswoj commented Sep 14, 2016 • edited Loading

anandthakker commented Sep 15, 2016

lucaswoj commented Sep 15, 2016 • edited Loading

anandthakker commented Sep 15, 2016

lucaswoj commented Oct 7, 2016

anandthakker commented Aug 11, 2016 •

edited

Loading

anandthakker commented Aug 24, 2016 •

edited

Loading

lucaswoj commented Sep 14, 2016 •

edited

Loading

lucaswoj commented Sep 15, 2016 •

edited

Loading