[WIP] raw buffer passthrough

[springmeyer]

Adds the ability to request a raw node.Buffer from the backend (rather than a mapnik.VectorTile object).

Motivation here is - when a raw buffer is needed, we want to avoid the performance overhead of the (slight cost) of creating a mapnik.VectorTile object and the (meaningful [benchmark needed]) cost of copying data back out of it when calling vtile.getData().

[flippmoke]

What happens if you have upgrade set to true and raw_buffer is also true? Should it default to still upgrading?

[springmeyer]

What happens if you have upgrade set to true and raw_buffer is also true? Should it default to still upgrading?

Good catch. If my memory serves me right the upgrade option we added in the case that we would want to auto-upgrade tiles in production from VT1 -> VT2. And I don't recall this option ever being used. So I think we should:

• assess if there are any downstream deps actually using this option
• if there are not any obvious deps using, them remove the option
• plan to publish a new dev version that removes the option
• NOTE: we cannot bump to 5.x because then we'd force downstream users to upgrade to node-mapnik 3.7.x which is not possible currently for the downstream usage of this module. So, we'll need to publish this branch as a 3.10.x-dev release.

[springmeyer]

So, we'll need to publish this branch as a 3.10.x-dev release.

Or publish as 5.x and bring in this change #156 such that we don't force downstream consumers of tilelive-vector into upgrading node-mapnik (when they can't).

[millzpaugh]

If my memory serves me right the upgrade option we added in the case that we would want to auto-upgrade tiles in production from VT1 -> VT2.

@springmeyer @flippmoke - Could we keep this conditional and use this to update all V1 tiles to V2 tiles? What would the performance hit be to check the version of each tile and update if it's V1?

[springmeyer]

@springmeyer @flippmoke - Could we keep this conditional and use this to update all V1 tiles to V2 tiles?
What would the performance hit be to check the version of each tile and update if it's V1?

@millzpaugh Are you asking if this update option could be used in the raw_buffer = true case? If so, I think that would complicate the code significantly since it would need to convert the raw buffer to a mapnik.VectorTile object and then convert back to a raw buffer. The performance hit could be significant. Perhaps that would not matter - in the sense that it would be called infrequently, however I feel like this is not the best place for v1 handling to happen (in your usecase). I think your usecase would be better handled by a) detecting a v1 tile (technically a tile with any single layer that is v1) and then b) running that through node-mapnik compositing (downstream from this code and not here).

[millzpaugh]

Are you asking if this update option could be used in the raw_buffer = true case?

@springmeyer yes, that's exactly what I was asking! This was also before Artem's workarounds in vtcomposite to handle V1 tiles, so now this is less relevant. Thanks for the clarification though.

Although - I'm also confused about next steps for this branch.

• assess if there are any downstream deps actually using this option

Does this entail checking all modules that have this listed as a dependency and then looking for upgrade usage? What if there are non-Mapbox projects using this option?

• if there are not any obvious deps using, them remove the option
• plan to publish a new dev version that removes the option
• NOTE: we cannot bump to 5.x because then we'd force downstream users to upgrade to node-mapnik 3.7.x which is not possible currently for the downstream usage of this module. So, we'll need to publish this branch as a 3.10.x-dev release.

Are we planning to use the dev release of this branch in production via mapbox-maps vtcomposite release?

[springmeyer]

@millzpaugh I retract my idea of removing this option. Rather I think that complicates this effort. Instead I think it is ideal to just leave it in and not worry about trying to remove.

So my new answer to this:

What happens if you have upgrade set to true and raw_buffer is also true? Should it default to still upgrading?

Is: lets not support upgrade:true with raw_buffer:true and therefore let's not worry about this. My decision here is colored by the fact that I consider tilelive-vector to be legacy software that should be fully replaced by other tools in the future. So not a lot of value to trimming its code if it is going to be replaced.

Are we planning to use the dev release of this branch in production via mapbox-maps vtcomposite release?

I think this is ready to merge and tag as an official new release, if you agree. So, please go ahead and do that when you have time and grab @mapsam for consultation on semver version to tag.

[springmeyer]

NOTE: we cannot bump to 5.x because then we'd force downstream users to upgrade to node-mapnik 3.7.x which is not possible currently for the downstream usage of this module. So, we'll need to publish this branch as a 3.10.x-dev release.

This is now no longer true since after this comment #156 was merged. Originally I put up #156 only as part of testing an upgrade to mapnik 4 in api-maps, but @ingalls needed it for another mapbox api, so this has already merged. What that means is that the next release that is made from this branch does not need to be a dev release and can rather be any semver version and downstream deps that need to stay using a given mapnik version can continue to do so by pinning the node-mapnik version. The npm logic should respect that pin and decide the right node-mapnik version to use, which will allow deduping to work. If deduping does not work, and downstream apps end up with multiple mapnik versions, that would be unexpected - the only thing that would explain it is if some dep other that tilelive-vector started requiring a more recent version than is pinned.

[springmeyer]

So, 👍 to tagging this as v4.2.0 per chat comment by @mapsam

[millzpaugh]

ok, great! I'm merging this in :) thanks @springmeyer & @mapsam