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

looks up file size in the header as well as last modified #35

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from

Conversation

jenningsanderson
Copy link
Collaborator

@jenningsanderson jenningsanderson commented Jul 24, 2020

Building on the previous update to lookup the "Last Modified" date, this also looks up the file size so that it doesn't get out of date.

@tyrasd - do you see any problems with this? It's similar to what you proposed before... is there any reason this might stop working?

Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, not all browsers consider the Content-Length HTTP header as CORS safe yet (Firefox version 78 for example: https://developer.mozilla.org/en-US/docs/Glossary/CORS-safelisted_response_header, see screenshot below). The header is however already in the fetch specs as a safelisted header: https://fetch.spec.whatwg.org/#cors-safelisted-response-header-name, so it will probably just take some time until it gets included in Firefox as well (I guess).

To make this work currently in all browsers, the S3 bucket would need to return the CORS expose-headers header in the result (i.e. Access-Control-Expose-Headers = Content-Length), as mentioned already in #26 (comment). Alternatively, one could just ignore null values in these cases and keep the default value (e.g. "40GB").

image

@tyrasd
Copy link
Member

tyrasd commented Jul 24, 2020

@dakotabenjamin can you set up the S3 bucket's ´Access-Control-Expose-Headers` CORS header?

@tyrasd tyrasd removed their assignment Jul 24, 2020
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 this pull request may close these issues.

2 participants