samvera / node-iiif

This module provides a full-featured IIIF Image API 2.1 and 3.0 image processor. It covers only the image processing pipeline itself, leaving input and output to the caller.
Apache License 2.0
27 stars 5 forks source link

Pass additional URL context to StreamResolver function #19

Closed blimmer closed 2 years ago

blimmer commented 2 years ago

Problem Description

Currently, the streamResolver only receives the id of the file which, in my particular case, doesn't provide enough context about the request to deterministically fetch the file from S3.

As a concrete example, here's the IIIF request URL:

https://my-lambda-name-12345678910.s3-object-lambda.us-west-2.amazonaws.com/subpath/de830e92-91cb-4b66-9daf-31eed3b0a1c7.jpeg/full/144,/0/default.jpeg

From the URL, you can see that a subpath is included in the URL. This is the subpath within my S3 bucket.

However, in my streamResolver function, I only get de830e92-91cb-4b66-9daf-31eed3b0a1c7.jpeg for the ID parameter, so I don't know where in the bucket to fetch the asset from.

Possible Solutions

It looks like the Processor class has a property, baseUrl, that I could infer the subpath from. Having access to this property would allow me to implement a StreamResolver that works with my bucket design.

Breaking Change

IMO, the most obvious API would be to change the id parameter to an object with more information about the requested object. For instance:

streamResolver({ id, baseUrl }, callback) { }

However, this would be a breaking change to the API. Folks who only used the id would need to make the following change:

streamResolver(id, callback) { }     // old
streamResolver({ id }, callback) { } // new

Non-Breaking Change

For a non-breaking change, we could pass the additional information as an additional parameter. For instance:

streamResolver(id, callback, { baseUrl }) { }

However, this is a bit weird because callback is currently an optional parameter, so you wouldn't be able to check the .length of the streamResolver method like you're doing today: https://github.com/samvera-labs/node-iiif/blob/2fc9dd88eff2b205a10f4af6acb17430c9e974f0/index.js#L79-L86

For this reason, I think the breaking change would be easier to implement, but that obviously has overhead for existing users.

Proceeding

I'd be happy to submit a PR for this, but I wanted to see what you think of this issue and what direction you might prefer to go.

Thanks for your time!

mbklein commented 2 years ago

Thanks for this. I think there's probably a way to implement the non-breaking change without disruption, but I'll have to chew on it a bit. If you want to submit a PR that bumps the major version and includes documentation on the backwards incompatibility, that'd probably work as well.

blimmer commented 2 years ago

Wow @mbklein - thank you so much for the fast response 😄. I was just about to write up a patch-package patch for the breaking change. I'd be happy to submit that as a PR with the major version bump and README updates. Thanks!

blimmer commented 2 years ago

Opened #20