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

use shorthash for data url image to prevent ENAMETOOLONG #12108

Merged
merged 8 commits into from
Oct 3, 2024

Conversation

lameuler
Copy link
Contributor

@lameuler lameuler commented Oct 3, 2024

Changes

currently data: urls are treated like normal urls and basename is used to get the filename to generate the cache and output files. however this can result in a very long filename depending on where the slashes (which are part of the base64 data) are, possibly resulting in errors.

to prevent this, can instead use a shorthash of the data url as the filename for data url images processed. so the final output image filename will be [hash of data url]_[hash].[ext] instead of [arbitrary basename of data url]_[hash].[ext]

Testing

added text fixture which processes data url images which would cause ENAMETOOLONG

Docs

should have no effect on user behaviour

Copy link

changeset-bot bot commented Oct 3, 2024

🦋 Changeset detected

Latest commit: 55f48bb

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Oct 3, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

The code looks good to me, but I let @Princesseuh double check

@ematipico ematipico requested a review from Princesseuh October 3, 2024 07:51
<Image src={data} alt="transparent" width="128" height="72" />
</div>
<div id="data-url-no-size">
<Image src={data} inferSize={true} alt="transparent" />
Copy link
Member

Choose a reason for hiding this comment

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

I honestly had no idea inferSize worked with data urls, pretty neat, though since we have all the data, we could make it so it's not required, ha.

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

Works with me, thank you!

This highlights a few flaws with the handling of data URIs, notably that they're considered remote images and so you need to add a remote pattern. Ideally, they'd be treated like some sort of in-between remote and local images and you wouldn't need any config, something for another PR, though.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@ematipico ematipico merged commit 918953b into withastro:main Oct 3, 2024
5 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants