Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass additional URL context to StreamResolver function #19

Closed
blimmer opened this issue Feb 17, 2022 · 3 comments · Fixed by #20
Closed

Pass additional URL context to StreamResolver function #19

blimmer opened this issue Feb 17, 2022 · 3 comments · Fixed by #20

Comments

@blimmer
Copy link
Contributor

blimmer commented Feb 17, 2022

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
Copy link
Member

mbklein commented Feb 17, 2022

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
Copy link
Contributor Author

blimmer commented Feb 17, 2022

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
Copy link
Contributor Author

blimmer commented Feb 17, 2022

Opened #20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants