Skip to content
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 #2648

Closed
wants to merge 7 commits into from

Conversation

anandthakker
Copy link
Contributor

@anandthakker anandthakker commented Jun 1, 2016

Replaces #2582

  • Inverts TilePyramid and Source dependencies. TilePyramid now essentially serves as a common wrapper class for dealing with sources.
  • Source types are now essentially:
(
  name: string,
  createSource: (id, options, dispatcher, onChange, callback: (err, source: Source) => void) => void
  ?workerSourceURL: string
)
  • And a Source is an object implementing:
id: string // <- it's a little weird to rely on sources presenting this as a property
tileSize: number
minzoom: number
maxzoom: number
// not sure about these three properties
roundZoom: boolean
reparseOverscaled: boolean
isTileClipped: boolean

load: (tile: Tile, cb: (err, ?data) => void) => void
abort: (tile: Tile) => void
unload: (tile: Tile) => void
serialize: () => Object // plain JS

Various todo's and questions:

  • The Tile objects are used by various source types to maintain tile-specific state; ideally, this implicit contract between TilePyramid and Source should be made explicit in the Source interface.
  • Decide on an approach to serialize() (see note here)
  • Source.is is only used in one place: to support passing already-created sources to addSource (and prevent the attempt to validate against the spec in that case). What's the best way to do this in the factory function approach, where it's a little weird to rely on instanceof?
  • Map methods (addSource, getSource). Related to this and the previous item: is it important to retain the ability to create sources without adding them to the map? If so, it might make the most sense to expose a createSource on the map obj, since creation now depends on a reference to dispatcher, which probably shouldn't be exposed.
  • It's weird to rely on sources presenting an id property with the correct value.
  • Reinstate query*Features
  • I'm not sure what the data yielded by load should look like. Currently, the only thing I can see being needed by code outside of the particular source is bucketStats, which the pyramid uses to emit the tile.stats event.

Anand Thakker added 7 commits June 1, 2016 13:49
 * Invert the TilePyramid / Source dependency
 * Change Source from class-based to factory-based interface
 * Add WorkerSource concept, allowing Source type to register worker-side functionality
 * Separate GeoJSONSource and ClusteredGeoJSONSource
return source ? Source._querySourceFeatures(source, params) : [];
},

addSourceType: function (name, sourceFactoryFn, workerSourceURL, callback) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucaswoj this could also be (name, { create: (id, options, dispatcher, onChange, callback) => void, ?workerSourceURL }, callback). Any preference between the two signatures?

@anandthakker
Copy link
Contributor Author

➡️ #2667

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant