-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Source interface to allow/prepare for extensibility #2667
Conversation
@@ -66,12 +72,36 @@ function Style(stylesheet, animationLoop) { | |||
this.glyphSource = new GlyphSource(stylesheet.glyphs); | |||
this._resolve(); | |||
this.fire('load'); | |||
|
|||
if (validateStyle.emitErrors(this, validateStyle(stylesheet))) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this move? Don't we want to continue to short circuit if there's a validation error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, yup - not quite sure when/how this happened, but it's a mistake. Thanks!
Regarding
|
Seems fine to me. The reason that |
@@ -68,10 +76,32 @@ function Style(stylesheet, animationLoop) { | |||
this.fire('load'); | |||
}.bind(this); | |||
|
|||
if (typeof stylesheet === 'string') { | |||
ajax.getJSON(normalizeURL(stylesheet), loaded); | |||
var sourceTypesLoaded = function(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this part of the code? It looks like it's loading support for core source types asynchronously? That seems odd to me, both that it should be async, and that something like addSourceType
should be the responsibility of Style
and attached as a non-underscored method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this comment tie in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfirebaugh Yeah, once this refactor settles, I think this async weirdness can be eliminated. It's here now only as a quick and dirty way to validate/show that addSourceType
works, but ultimately there's no reason that core source types need to be added asynchronously through the addSourceType
interface.
I do think addSourceType
should:
- (a) be a public, non-underscored method -- because its purpose is to allow third-party source types can be plugged in, which is the motivating reason for this PR; and
- (b) be async -- because if a source type needs to provide a WorkerSource implementation, then the process of having all the workers load that implementation via
importScript
is async.
I'm still not sure if we want to have |
@mourner I definitely see what you're saying, but I'm not 100% sold on the idea that different source types should only map to the format of the original source data being used. I agree that clustering is an interpretation of the data; but then would hex-bins and square-bins also just be interpretations that belong as options on GeoJSONSource? |
|
||
if (source.id !== id) { | ||
throw new Error('Expected Source id to be ' + id + ' instead of ' + source.id); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think it probably just makes more sense to do source.id = id
here.
c1c041d
to
2773f2f
Compare
@lucaswoj @jfirebaugh @mourner Updated the top of the ticket with more complete description of the overall design and capturing outstanding questions/tasks from the reviews thus far (so we should now be okay to rebase w/o losing key discussion points). |
Transcribing from a diff comment: @jfirebaugh : [It] seems odd to me, both that it should be async, and that something like addSourceType should be the responsibility of Style and attached as a non-underscored method. @anandthakker : I do think addSourceType should:
|
We should validate the custom source architecture and build some demo sources before documenting the internals publicly.
39b7711
to
ad2e938
Compare
🎉 |
@lucaswoj For some reason, the build fails in master since the merge: https://circleci.com/gh/mapbox/mapbox-gl-js/4209 |
Thanks for the heads-up @mourner. I'll try to resolve ASAP. |
@mourner how can one now configure clustering as was possible before i.e.
|
@hdrdavies same as before, as you can see on the example https://www.mapbox.com/mapbox-gl-js/example/cluster/ |
@mourner thanks very much!! It seems I'm getting other strange constructor errors even though I'm not using Do you think you guys will be adding support for the clustering of custom HTML markers made using |
@hdrdavies currently no plans to do this for the HTML markers — this would be a non-trivial effort. Perhaps someone will contribute a plugin. |
Thanks @mourner. @hdrdavies, you might be able to write your own HTML clustering code using the same backend as GL JS https://github.com/mapbox/supercluster |
Replaces #2648
TilePyramid
andSource
dependencies.TilePyramid
now essentially serves as a common wrapper class for dealing with sources.TilePyramid
toSourceCache
. This is partly aspirational, and partly to make it more reasonable to have things calledsource
that are actually TilePyramids 😁Source
independently of aMap
instance - i.e., no morenew mapboxgl.GeoJSONSource()
; sources must be created withmap.addSource(id, json)
.(id, options, dispatcher) => Source
and an optionalworkerSourceURL
static propertySource
provided by the factory function is an object implementing:WorkerSourceURL
points to a script, which, when executed in the worker thread, must callself.registerWorkerSource(WorkerSource)
, whereWorkerSource
is a class that the worker will construct withnew WorkerSource(actor, styleLayers)
.WorkerSource instances may implement a
loadTile
method; if they do, then this will be used by the worker when its'load tile'
target is invoked with atype: 'source-type'
parameter (e.g.type: 'geojson'
). Any other methods onworkerSource
instance can be targeted by the main-threadSource
viadispatcher.send('source-type.methodname', params, callback)
.Questions:
Tile
objects are used by various source types to maintain tile-specific state; ideally, this implicit contract betweenTilePyramid
andSource
should be made explicit in the Source interface... but I think this is perhaps out of scope for this particular PR - perhaps this would be naturally addressed in Implement cross-source symbol placement #2703, or else could be ticketed separately?data
yielded byloadTile
should look like. Currently, the only thing I can see being needed by code outside of the particular source isbucketStats
, which the pyramid uses to emit thetile.stats
event. @lucaswoj can you weigh on in this?Remaining Tasks:
Source
interfaceWorkerSource
addSourceType
-- includes documenting thecreate
function and theself.registerWorkerSource(...)
requirement onWorkerSource
modules.Source
,WorkerSource
, andaddSourceType
. Perhaps an example of a 3rd-party source type would suffice. update: went with a link to https://github.com/developmentseed/mapbox-gl-topojson in the docs forGeoJSONWorkerSource
.loadTileJSON
,queryRenderedFeatures
, andquerySourceFeatures
out ofsource.js
TODO