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.

Sign up for GitHub

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

[@astrojs/image]: Fix <Picture /> component with query string #4997

Merged
merged 6 commits into from
Oct 6, 2022
Merged

[@astrojs/image]: Fix <Picture /> component with query string #4997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
83b90c9
remove query params from file extension
panwauu Oct 6, 2022
353d770
Revert changes to make change as small as possible
panwauu Oct 6, 2022
394c9b1
moving the removeQueryParam check to basename()
Oct 6, 2022
1bbc8c1
chore: adding a changeset
Oct 6, 2022
1958d91
Merge branch 'main' into enable_picture_with_query_params
Oct 6, 2022
859ddf4
adding SSR test coverage for new picture test cases
Oct 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/large-bananas-prove.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@astrojs/image': patch
---

Fixes a bug that lost query parameters for remote images in the `<Picture />` component
4 changes: 2 additions & 2 deletions packages/integrations/image/src/utils/paths.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export function extname(src: string) {
return '';
}

return src.substring(src.length - (base.length - index));
return base.substring(index);
}

function removeExtname(src: string) {
Expand All @@ -32,7 +32,7 @@ function removeExtname(src: string) {
}

function basename(src: string) {
return src.replace(/^.*[\\\/]/, '');
return removeQueryString(src.replace(/^.*[\\\/]/, ''));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the bug fix, basename should be stripping off query params

}

export function propsToFilename(transform: TransformOptions) {
Expand Down
0 ...ure/src/assets/blog/introducing-astro.jpg → ...ure/src/assets/blog/introducing astro.jpg
View file
Open in desktop
File renamed without changes
5 changes: 5 additions & 0 deletions packages/integrations/image/test/fixtures/basic-picture/src/pages/index.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
import socialJpg from '../assets/social.jpg';
import introJpg from '../assets/blog/introducing astro.jpg';
import { Picture } from '@astrojs/image/components';
---

Expand All @@ -10,6 +11,8 @@ import { Picture } from '@astrojs/image/components';
<body>
<Picture id="hero" src="/hero.jpg" sizes="100vw" widths={[384, 768]} aspectRatio={768/414} alt="Hero image" />
<br />
<Picture id="spaces" src={introJpg} sizes="100vw" widths={[384, 768]} aspectRatio={768/414} alt="spaces" />
<br />
<Picture id="social-jpg" src={socialJpg} sizes="(min-width: 640px) 50vw, 100vw" widths={[253, 506]} alt="Social image" />
<br />
<Picture id="google" src="https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_272x92dp.png" sizes="(min-width: 640px) 50vw, 100vw" widths={[272, 544]} aspectRatio={544/184} alt="Google logo" formats={["avif", "webp", "png"]} />
Expand All @@ -19,5 +22,7 @@ import { Picture } from '@astrojs/image/components';
<Picture id="bg-color" src="https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_272x92dp.png" sizes="(min-width: 640px) 50vw, 100vw" widths={[272, 544]} aspectRatio={544/184} alt="Google logo" background="rgb(51, 51, 51)" formats={['avif', 'jpeg']} />
<br />
<Picture id="ipsum" src="https://dummyimage.com/200x300" sizes="100vw" widths={[100, 200]} aspectRatio={2/3} formats={["avif", "webp", "jpg"]} alt="ipsum" />
<br />
<Picture id="query" src="https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_272x92dp.png?token=abc" sizes="100vw" widths={[544]} aspectRatio={544/184} alt="query" />
</body>
</html>
29 changes: 28 additions & 1 deletion packages/integrations/image/test/picture-ssg.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ describe('SSG pictures - dev', function () {
query: { f: 'jpg', w: '506', h: '253' },
alt: 'Social image',
},
{
title: 'Filename with spaces',
id: '#spaces',
url: '/@astroimage/assets/blog/introducing astro.jpg',
query: { f: 'jpg', w: '768', h: '414' },
alt: 'spaces'
},
{
title: 'Inline imports',
id: '#inline',
Expand Down Expand Up @@ -116,6 +123,13 @@ describe('SSG pictures with subpath - dev', function () {
query: { f: 'jpg', w: '506', h: '253' },
alt: 'Social image',
},
{
title: 'Filename with spaces',
id: '#spaces',
url: '/@astroimage/assets/blog/introducing astro.jpg',
query: { f: 'jpg', w: '768', h: '414' },
alt: 'spaces'
},
{
title: 'Inline imports',
id: '#inline',
Expand Down Expand Up @@ -199,6 +213,13 @@ describe('SSG pictures - build', function () {
size: { width: 506, height: 253, type: 'jpg' },
alt: 'Social image',
},
{
title: 'Filename with spaces',
id: '#spaces',
regex: /^\/assets\/introducing astro.\w{8}_\w{4,10}.jpg/,
size: { width: 768, height: 414, type: 'jpg' },
alt: 'spaces',
},
{
title: 'Inline images',
id: '#inline',
Expand Down Expand Up @@ -244,7 +265,13 @@ describe('SSG pictures - build', function () {
const srcset = source.attr('srcset');

for (const src of srcset.split(',')) {
const [pathname, width] = src.split(' ');
const segments = src.split(' ');

// filenames may have a space in them, pop the last item for the
// width and join the other segments back for the filepath
const width = segments.pop();
const pathname = segments.join(' ');

const widthNum = parseInt(width.substring(0, width.length - 1));

verifyImage(pathname, {
Expand Down
14 changes: 14 additions & 0 deletions packages/integrations/image/test/picture-ssr-build.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@ describe('SSR pictures - build', function () {
query: { f: 'jpg', w: '506', h: '253', href: /^\/assets\/social.\w{8}.jpg/ },
alt: 'Social image',
},
{
title: 'Filename with spaces',
id: '#spaces',
url: '/_image',
query: { w: '768', h: '414', f: 'jpg', href: /^\/assets\/introducing astro.\w{8}.jpg/ },
alt: 'spaces',
},
{
title: 'Inline imports',
id: '#inline',
Expand Down Expand Up @@ -127,6 +134,13 @@ describe('SSR pictures with subpath - build', function () {
query: { f: 'jpg', w: '506', h: '253', href: /^\/docs\/assets\/social.\w{8}.jpg/ },
alt: 'Social image',
},
{
title: 'Filename with spaces',
id: '#spaces',
url: '/_image',
query: { w: '768', h: '414', f: 'jpg', href: /^\/docs\/assets\/introducing astro.\w{8}.jpg/ },
alt: 'spaces',
},
{
title: 'Inline imports',
id: '#inline',
Expand Down
16 changes: 16 additions & 0 deletions packages/integrations/image/test/picture-ssr-dev.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,14 @@ describe('SSR pictures - dev', function () {
contentType: 'image/jpeg',
alt: 'Social image',
},
{
title: 'Filename with spaces',
id: '#spaces',
url: '/@astroimage/assets/blog/introducing astro.jpg',
query: { w: '768', h: '414', f: 'jpg' },
contentType: 'image/jpeg',
alt: 'spaces',
},
{
title: 'Inline imports',
id: '#inline',
Expand Down Expand Up @@ -154,6 +162,14 @@ describe('SSR pictures with subpath - dev', function () {
contentType: 'image/jpeg',
alt: 'Social image',
},
{
title: 'Filename with spaces',
id: '#spaces',
url: '/@astroimage/assets/blog/introducing astro.jpg',
query: { w: '768', h: '414', f: 'jpg' },
contentType: 'image/jpeg',
alt: 'spaces',
},
{
title: 'Inline imports',
id: '#inline',
Expand Down