Promisify tilelive and add extra context

[nyurik]

tilelive is fundamentally a URI-based dependency container service, allowing separation between service implementation/instantiation and service consumption. Moving away from the rigid getTile(z,x,y) API would allow more diverse context parameters and non-tile services.

• A single "get" function with a single options object instead of individual parameters to handle implementation-specific functionality.
• Promises instead of callbacks, as Promises have now became widely spread, and the current Node (7) supports language-integrated constructs (async/await)

For example, getTile(z, x, y, callback(err, data, headers)) would be something like Promise<{data, headers}> getAsync({zoom: z, index: ind, extra1: ...})

For compatibility, tilelive should inject a rudimentary getAsync() (See injection lib), that only supports the basic {tile|grid|info} modes into the legacy tilelive sources. The auto-injected getAsync() would not support any of the magical customizations, such as parameters set on callback function. For that, the source would have to implement its own version of getAsync, e.g. tilelive-vector PR.

Immediate benefits of adding a single getAsync() function:

• ability to pass additional parameters through the stack callchain. E.g. language, licence, ...
• ability to handle non-tile requests like snapshots (custom area of the map with the given width/height)
• ability to integrate related services using the same pipeline. For example, in Wikipedia, Kartotherian can get geojson for a specific area, and that same geojson could be used as an extra layer in the snapshot service. Another example - simple-style markers (pushpins) - request a given marker/color/icon size/scaling, and get back an image via the same chain, but with somewhat different params. The source instantiation will continue using the existing tilelive container, without the need to create a separate pipeline.
• ability to introduce an alternative (additional) way of addressing tiles - a single quad-based index instead of x,y coordinates

[nyurik]

I have implemented a library, tests and all, to dynamically supply the getAsync() method to the existing tilelive sources, or for the newer sources, it can inject the getTile/getGrid/getInfo. https://github.com/kartotherian/tilelive-promise#tilelive-promise

[mojodna]

👎 on Promises (at least until async/await is mainstream), especially since callback-driven interfaces can be adapted to work with Promises fairly easily.

👍 on options. tilelive-vector demonstrates the need for being able to wedge in custom parameters when requesting tiles w/ non-standard sizing, etc.

+0 on batching, since it already exists in a couple of different forms.

Can you elaborate on the quad-based index idea? z/x/y URLs are very common, so I'd be hesitant to move away from that without good reason.

I've implemented one mode of of batching w/ tilelive-streaming (tilelive itself has a different streaming interface that also supports batch operations).

One longstanding (slightly voiced) request I have is for getTile and putTile to support streams directly (included as an after-market enhancement in tilelive-streaming). I would love to be able to call src.getTile(x, y, z).pipe(res) to pipe a tile direct to an output response (possibly using multiple chunks). Most extant tilelive sources don't read/generate chunkable output, but I think that's a bad assumption to make.

[flippmoke]

@rclark -- you would probably be the best person to weigh in here?

[nyurik]

@mojodna, thanks for the response!

• I think Promises are highly mainstream already, and I think it makes no sense to do "two step migration" - first introduce a callback interface, and later introduce yet another Promise-based version. We might as well just add one more function that returns a Promise - much cleaner for newer usages, while compatible with existing code.
• Batching - agree, not as urgent, might be a separate discussion.
• Streaming: the tilelive-streaming looks interesting. A while ago I also implemented promistreamus - a way to treat a stream as individual waitable objects. Perhaps streaming is also outside of this discussion as it has been mostly solved.
• Re getTile().pipe(res) - I like the idea, it could be very beneficial for large getAsync() responses to be chunked - e.g. for getting a large snapshot image with multiple tiles glued together. But your version assumes that every getTile() would return a stream. I think we should introduce a generic response object into the API -- instead of response containing data, it would have a getData() and getDataStream() funcs: getAsync({x,y,z}).then(result => result.getDataStream().pipe(res)). The response object would also handle auto-conversion - so if the source provider only deals with complete tiles, the response object would fake the getDataStream().

[nyurik]

@mojodna the above PR implements my proposal for the tilelive-vector -- no more callback hackary: source.getAsync({z,x,y, scale: 1.5}). The added bonus is the parameter pass-through -- all parameters are passed to the source of the data, which means that a license code could be sent all the way to the storage system without any prior checks.

[rclark]

Overall I'm pretty enthusiastic about attempts to refresh the tilelive API contract, though ideally in ways that avoid any breaking API changes. This could include:

• optional options objects -- tilelive-vector and so many other examples are out there where the lack of such an argument leads to very "creative", confusing, hidden passing of configuration details.

• methods like .getTile() could return a promise, or an object with a .promise() method (this being a pattern I'm quite accustomed to from the aws-sdk-js), but also continue to support a callback interface. A return object for .getTile() could also return a .createReadStream() method to handle what @mojodna is after?

... but we should avoid changing the function signature to accept like a quadkey or something other than z, x, y. Do we need to introduce an entirely new method? Or can we retrofit the .getTile() and .putTile() methods in a non-destructive way?

[nyurik]

@rclark, the additional getAsync() function is fully non-breaking -- that's why I implemented tilelive-promise -- it adds getAsync to the existing implementations, without changing existing signatures. I think adding just that one function will solve most of our goals here: promise-based, options object as the way to pass in any parameters. I agree that we we should not change the existing getTile/Grid/Info signatures. The quad-tile index is just an extra param (optional) inside getAsync. It can be a requirement for the implementers of getAsync, but it is not required for any legacy implementers for getTile, because the above library automatically takes care of the conversion.

[rclark]

If you'd prefer to keep these functionalities contained within the tilelive-promise module, then should we close the ticket here?

[nyurik]

@rclark we could move that functionality into tilelive itself - this way the tilelive.load would automatically bring the interface to the common denominator, without any custom logic inside each module.

[rclark]

Ok, if we're going to talk about bringing this functionality into tilelive itself, a couple of points to consider:

• .getAsync() is not a very descriptive name -- in fact all the API methods are asynchronous. I would lean more in the direction that I outlined where the .getTile() method actually returns an object that can provide promise, streams, etc.

• The kinds of configuration options that various tilelive-* modules accept, and how they accept them, is a tremendous mess. I'm not sure that tilelive itself (and .load() specifically) can be expected to accept an options object and map those options into the correct way to call a module-level function. Instead, I think you have to update this API contract document and explicitly specify how implementation-specific options like these should be provided to a getTile request.

These changes would adjust the existing API interface, but could be handled in a non-breaking fashion. For the options object, it would still have to be up to the implementation (eg. tilelive-vector) to support it.

[nyurik]

• The *Async() is really a Promise, rather than callback convention. I think it started in .NET, but the best known Promise library Bluebird injects matching *Async functions during promisification of the existing callback-based ones. Of course naming is a hard problem :)
• The reason for get as oppose to get{Tile|Grid|Info} is because I think tilelive should be extended beyond the "tile" concept. Tilelive is basically a dependency container - a URI-IDed repository of auto-instantiated services. As such, clients may choose to register other map-related services, such as maki icon generator, or a snapshot (abaculus-type) tile gluing service, making it into a true lego-like on-the-fly pipeline assembly.
• I am not proposing for the .load() to accept any of these options. I do not want auto-magical conversion of the old interface to the new one. If getTile() uses some magic callback-attached parameters, the users of that module already has a custom code to call that getTile() directly, and the auto-injected getAsync() would not support any of it. The module would have to implement its own version of getAsync(), like I did for tilelive-vector in my pull request to extend beyond the basic {type,z,x,y} parameters.

[yhahn]

The reason for get as oppose to get{Tile|Grid|Info} is because I think tilelive should be extended beyond the "tile" concept.

No -- let's split this out/defer this discussion to some occasion where this concretely matters. @nyurik can you focus this discussion on adding a backwards-compatible Promise interface?

[nyurik]

@yhahn at a bare minimum, we should add two features at once - to avoid introducing two new interfaces that will both need to be supported: Promisification, and extra parameter passing. As discussed with @rclark, changing the existing getTile & getGrid would break existing contracts. For example, getTile().promise() means that instead of returning undefined, we would return an object - which might be unexpected in some cases, when existing code does return getTile(z,x,y,callback);. Changing the signature of the existing getTile is also bad, as it might break for existing implementations. So my proposal - getAsync() - is fully backward's compatible, as it can be polyfilled by tilelive itself.

@rclark had a valid objection that removing z,x,y parameters would imply "get anything", which might be out of scope for tilelive. I am not yet sure this is true, as my hope was to introduce the minimal number of changes, and we need to cover all 3 functions, and getInfo does not have x,y,z. On the other hand, getInfo can be treated as immutable, without any extra options, so we can skip it for this discussion and only look at getTile & getGrid. But in that case, I don't think we should consider them as two distinct functions - getGrid is basically getTile with the utf format parameter according to tilelive-vector -- so we can really treat them the same once we introduce the options param.

To sum up, how about Promise<{data, headers, ...}> getTilePromise(z, x, y [,options])? This way we avoid conflicting with the existing auto-Promisificated code which adds getTileAsync(), we keep the z,x,y mandatory params for simple usages, and we have an optional extra parameter to specify options. getGrid could be implemented as format=utf or similar. Or we can just put z,x,y inside the options, making it the only required parameter: getTilePromise({z,x,y,...})

[yhahn]

For example, getTile().promise() means that instead of returning undefined, we would return an object - which might be unexpected in some cases, when existing code does return getTile(z,x,y,callback);.

Could we take this approach but flag this as a potentially breaking change by doing a semver major version bump? That way developers doing the major version bump will probably have a painless upgrade experience unless they have come to expect an undefined return value (probably a minority/corner case)?

[nyurik]

Could we take this approach but flag this as a potentially breaking change by doing a semver major version bump? That way developers doing the major version bump will probably have a painless upgrade experience unless they have come to expect an undefined return value (probably a minority/corner case)?

I think this is not very good - the whole idea behind tilelive is "plug and play" components. You are basically saying that every single implementation of tilelive api must be verified to be handling the return correctly before you can use it, whereas with my proposal, all existing code will work without a single change to any of the components. I am already using it in Kartotherian with the injection lib, and the only time I need to modify existing code is when I want to change its behavior.

[jingsam]

As a developer that uses tilelive in production, I don't see any obvious benefits using a promisify interface instead of callback. Async/await is promising but far from mature for production usage, especially for modules that rely on native c++ addons such as node-mapnik which only provide precompiled binary up to node 6, and node-mapbox-gl-native sticks to node 4. Promise also provides unnecessary abstraction that may decrease performance，while performance matters as tilelive mostly is used on the backend services.

If you likes promise, as @mojodna said, there are too many libraries which can help you promisify any callbacks. getTile(z, x, y) returns a promise while getTile(z, x, y, callback) executes immediately would be better, but many tilelive-modules throws an error if callbacks are not provided.

getAsync() is a bad idea because: 1) It is to implicit to infer what the functionality of this api; 2) getAsync() is trying to "get everything", which is obey the principle of KISS.

Tilelive is a kind of contract or protocol, so please keep it stable and simple. Please do not introduce any incompatible changes unless we have concrete reasons that we must change it.

[rclark]

You are basically saying that every single implementation of tilelive api must be verified to be handling the return correctly before you can use it

Yeah, this is a bummer, and this is one reason why I think that the best approach is the one you've already implemented: https://github.com/kartotherian/tilelive-promise -- a helper library that can wrap tilelive modules and expose the functionality that you want to have, but that might not be important to others.

Tilelive is a kind of contract or protocol

I'm very much in agreement with this statement. Though I've always been bothered by the way this contract is not actually enforced on tilelive-* modules. Figuring out ways to move away from the implicit contract is a direction that I'd love to explore elsewhere.

As it stands now, I'm hesitant to add JS logic to tilelive.js that would wrap module functions like getTile, etc. Tilelive.js as it stands now is about defining this implicit contract, and about assistance loading modules. The present exception to that statement are the streaming functions, which honestly I would like to see move out of tilelive.js into an independent module.

[nyurik]

Tilelive.js as it stands now is about defining this implicit contract, and about assistance loading modules.

@rclark I so wish we can separate these two. API and dependency container are really two separate. unrelated things. And yes, stream functionality might not belong there either...

There is one fundamental problem that I cannot work around with the current code. Given an API implementation, like tilelive-vector, that uses an instance of another implementation (the backend), there is currently no way to pass parameters through the layers. I had to patch tilelive-vector itself to allow "context passing". Without it, there is no way for a license key, user's language, or any other information to be passed.

My getAsync polyfill library obviously does not address that, as it can only convert (opts) into {x,y,z}.