mojodna / tilelive-http

An HTTP source for tilelive
MIT License
12 stars 10 forks source link

Not working with tilelive.auto #5

Closed gravitystorm closed 10 years ago

gravitystorm commented 10 years ago

I was trying to get tilelive-http working with tilelive.auto, but it wasn't registering. A small amount of investigation got the following error (trimmed for clarity):

[TypeError: Object function (tilelive, options) {
  var HttpSource = function(uri, callback) {
    this.source = url.format(uri);
...
  return HttpSource;
} has no method 'registerProtocols']

I took a stab at rewriting the module using tilelive-file and mbtiles as a guide, sort of 'unwrapping' the definitions into:

module.exports = HttpSource;

function HttpSource(uri, callback) {
     this.source = url.format(uri);
 ...

Full results at https://gist.github.com/gravitystorm/d64b6efee6b8b3b8f29c . It seems to work for my usecase, but I've no idea about the wider implications of the approach.

mojodna commented 10 years ago

I've been favoring explicit requireing of tilelive modules over auto loading, so I haven't look into that much. It doesn't surprise me, as most of the tilelive modules I've written export a factory method that requires a tilelive argument instead of a class (like tilelive-file, mbtiles, etc.).

My approach is an attempt to prevent modules from needing to declare a dependency on tilelive itself (when they need to resolve references to other providers) in favor of using a single, shared instance (therefore shared registry, which is really the core issue). (It also allows tilelive to be swapped out with lookalikes, such as tilelive-cache and tilelive-streaming, which aim to transparently add functionality.

https://github.com/mapbox/tilelive-vector/pull/49 has some more background on the issue.

This approach originated when I discovered that using npm link to facilitate module debugging resulting in multiple instances of tilelive floating around, each with different registries.

As for your rewrite, it's largely right, except for https://gist.github.com/gravitystorm/d64b6efee6b8b3b8f29c#file-index-js-L20, which refers to an undefined tilelive instance.

I should probably look at submitting a patch for tilelive.auto that looks something like https://github.com/mojodna/tl/blob/master/lib/commands/copy.js#L49-L57

mojodna commented 10 years ago

Patch submitted as mapbox/tilelive.js#75.

It also struck me that a workaround for this problem would be to implement registerProtocols such that it can capture the reference to tilelive when first called, but that's ugly.

mojodna commented 10 years ago

This was fixed in tilelive-5.0.2.