stadiamaps / pmtiles-rs

Rust implementation of PMTiles
Apache License 2.0
54 stars 8 forks source link

Add support for `rust-s3` backend. #30

Closed lseelenbinder closed 8 months ago

lseelenbinder commented 8 months ago

Fixes #11 and could be used to address https://github.com/maplibre/martin/discussions/1125.

lseelenbinder commented 8 months ago

looks good! A few minor nits, and we need to re-export the underlying s3 crate somehow, but it conflicts with the s3.rs file. I would recommend NOT exporting the s3, http, and mmap namespaces. Instead, publish their content directly.

// --- this is lib.rs ---

pub use s3;  // re-export s3 crate as pmtiles::s3

mod s3_backend;
pub use s3_backend S3Backend;

mod http;
pub use http::HttpBackend;

Do we want to re-export s3 or allow users to bring their own (as we do with reqwest)? I tried really hard to make this TLS implementation agnostic, but rust-s3 doesn't build properly when it doesn't have a concrete credentials type.

lseelenbinder commented 8 months ago

There is really no such thing as "bring your own" here. If we do not re-export a dependency, users must play the magic game of "match my dependencies with the dependencies of my dependency" - i.e. user must use the same version of reqwest lib as used by the API of the pmtiles crate. This is an antipattern because it will only work if Cargo ends up using identical version of reqwest for both. Otherwise, it might not compile. That's why sometimes people opt to use reqwest = "*" - which again is an antipattern, but they have to do it to match the API of a crate they want to use.

Instead, I think a much safer way is to either (1) ensure all pmtiles crate API only uses standard Rust types (or types declared in the pmtiles crate itself), or (2) re-export all 3rd party types together with all the other stuff related to those types - i.e. re-export reqwest and s3 sub-dependencies in their own namespaces like pmtiles::reqwest:: and pmtiles::s3::.

Fair enough. And users can always bring their own, if they want—just because we're nice and re-export doesn't mean users will use the exports. 😄