-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix: encode spaces when generating srcset
#7340
Conversation
🦋 Changeset detectedLatest commit: 57af68b 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 |
@@ -85,7 +85,7 @@ export async function getPicture(params: GetPictureParams): Promise<GetPictureRe | |||
image = img; | |||
} | |||
|
|||
return `${img.src} ${width}w`; | |||
return `${img.src?.replaceAll(' ', '%20')} ${width}w`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK encoding the whole img.src
was not required. I've tested the filename @withastro:안녕하세요🚀+❤.jpg
which contains an emoji, URI reserved characters, and Korean chracters but it seemed okay before (and also after) applying this.
The code looks great to me (feel free to use encodeUri though if it does fix the problem, that way we're fully covered!), however the testing strategy is very heavy-handed and we've been trying to improve our testing strategy recently. I'd suggest not using a full E2E test for this and instead just adding a test to this file: https://github.com/withastro/astro/blob/main/packages/integrations/image/test/picture-ssg.test.js |
@Princesseuh Yeah, agreed and understood. Actually I've also updated |
I think we can remove playwright entirely with the new test. I'm also slightly leaning towards |
Changes
getPicture
may generate invalidsrcset
values after SSR #7338.HELLO WORLD.png
currently will havesrcset
ofHELLO WORLD.png 100w
.source
s are ignored andimg
'ssrc
will be rendered.console.warn
calls.Testing
packages/integrations/image/test/picture-ssg.test.js
packages/integrations/image/e2e/picture.test.js
basic-picture
fixture?Docs
Picture
andgetPicture
with space characters in filenames.