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

Jump to bottom

WSTEAMA-749: Update service worker tests and URL generation logic #11595

Merged
merged 16 commits into from
May 9, 2024
Merged

WSTEAMA-749: Update service worker tests and URL generation logic #11595

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
178a0f1
Ensure headers are passed to the Request object
alex-magana May 3, 2024
9c6308e
Match .jpg.webp and .png.webp
alex-magana May 3, 2024
a448df1
Merge branch 'WSTEAMA-749-implement-webp-support-for-all-images-in-si…
alex-magana May 6, 2024
cdee302
Update service worker URL filter and associated tests
alex-magana May 6, 2024
fb91fcd
Add URL checks for adding .webp extension
alex-magana May 6, 2024
b220666
Fix amz regex
alex-magana May 6, 2024
6c4dc62
Use nono-capture group for performance since only matches are required
alex-magana May 6, 2024
af64a4b
Add tests for ichef URL generation
alex-magana May 7, 2024
219514c
Remove redundant tests
alex-magana May 7, 2024
de0b225
Ensure amz/worldservice does not apply to the news subdomain
alex-magana May 7, 2024
be69d17
Refine web support check to exclude amz/worldservice for news via regex
alex-magana May 7, 2024
b4a75b7
Ensure .webp is not appended to the URL if already included
alex-magana May 7, 2024
097b2af
Add guide comment
alex-magana May 7, 2024
dbc1398
Use the pattern in the service worker to match .webp images
alex-magana May 8, 2024
5a30985
Change service worker to match the images ichef project
alex-magana May 9, 2024
d2157c5
Set sad path tests in an isolated block
alex-magana May 9, 2024
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
4 changes: 3 additions & 1 deletion public/sw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ self.addEventListener('install', event => {

const fetchEventHandler = async event => {
if (
/^https:\/\/ichef(\.test)?\.bbci\.co\.uk\/.+\.webp$/.test(event.request.url)
/^https:\/\/ichef(\.test)?\.bbci\.co\.uk\/(news|ace\/(standard|ws))\/.+(\.jpg|\.png).webp$/.test(
Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking about how there are some images with different URL types we may also need to put .webp on in other areas of the code, and then remove it here in cases of it not being supported:

Images like the podcast promo image for example like https://ichef.bbc.co.uk/images/ic/512x512/p0htdh6t.png which we want to be https://ichef.bbc.co.uk/images/ic/512x512/p0htdh6t.png.webp. This one wouldn't fall into this condition, and so we wouldn't remove .webp in the cases webp isn't supported.

Copy link
Contributor Author

@alex-magana alex-magana May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To resolve this issues, do we want to use the less specific pattern below
/^https:\/\/ichef(\.test)?\.bbci\.co\.uk\/.+\.webp$/
or expand the URL checks with something along this lines to cover each
scenario we need to support?
The limitation presented by a less specific pattern is the likelihood of matching
.webp URLs that we aren't aware of i.e we don't know where the .webp is being set.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have looked throught the codebase and I don't see webp being set anywhere else, and it shouldn't be .webp coming from Ares. Maybe this will change when we do some of the work in the BFF to add webp in there, but for now it seems that it will be ok to make the pattern less specific to catch all ichef urls that have webp added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me.
Thanks for looking looking into this concern. With this knowledge, we know we won't
encounter any surprises.
I'll change the pattern to only check for .webp at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed this to (news|images|ace\/(standard|ws)) to

  • retain the rationale behind the specificity introduced to the matching pattern here
  • ensure we're able to keep out these wrong paths in the URL.
  • to ensure the regex pattern remains performant by being able to weed out unsupported URLs based on the image path.
    Do let me know if that's okay.

event.request.url,
)
) {
const req = event.request.clone();

Expand Down
13 changes: 9 additions & 4 deletions src/app/lib/utilities/ichefURL/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import { getEnvConfig } from '../getEnvConfig';

const webpSupportedPatterns = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this mean that according to '//ace/(?:standard|ws)/.+(?:/amz/worldservice/)?.*/', if the url has ace/standard and amz/worldservice then it will always have .webp added to the end, even if it already has. webp on the end? Do some of the ace/standard and amz/worldservice urls ever already have .webp on the end? Just because if we add .webp to one that already has .webp on the end it makes a bad url. Otherwise this looks good! But just checking that scenario. I see the other IRL in this array doesn't have that problem because it specifies .jpg or .png on the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just spoke to @eagerterrier and the URLs won't already have webp on. It is only added in this buildIChefUrl file and the service worker. So this should be ok!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brill.
I've added this test to ensure we're not appending .webp if it's already in the URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As regards this query, since the matching pattern checks that the URL ends with .jpg or .png as per (?:\.jpg|\.png), we wouldn't append .webp to the URL if already exists.
This is because this pattern will return false when we check for a match.

/^https:\/\/ichef(?:\.test)?\.bbci\.co\.uk\/(?:news|ace\/(?:standard|ws))\/.+(?:\.jpg|\.png)$/,
/\/ace\/(?:standard|ws)\/.+(?:\/amz\/worldservice\/)?.*/,
];

const isSupportedWebpUrl = url =>
webpSupportedPatterns.every(pattern => pattern.test(url));

const buildPlaceholderSrc = (src, resolution) => {
const imageSrc =
src || 'https://ichef.bbci.co.uk/images/ic/640xn/p0b36kgx.png';
Expand Down Expand Up @@ -35,10 +43,7 @@ const buildIChefURL = ({ originCode, locator, resolution }) => {
.filter(Boolean)
.join('/');

return url.endsWith('.webp') ||
(url.includes('amz/worldservice') && !url.includes('ace/standard'))
? url
: `${url}.webp`;
return isSupportedWebpUrl(url) ? `${url}.webp` : url;
};

export default buildIChefURL;
43 changes: 19 additions & 24 deletions src/app/lib/utilities/ichefURL/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,32 +13,27 @@ describe('getIchefURL', () => {
expect(getIChefURL(input)).toEqual(expectedOutput);
});

it('builds WebP ichef img url based on originCode, locator, resolution and isWebP passed', () => {
const input = {
originCode: 'cpsprodpb',
locator: 'cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.jpg',
resolution: '660',
isWebP: true,
};
const expectedOutput =
'https://ichef.bbci.co.uk/ace/ws/660/cpsprodpb/cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.jpg.webp';

expect(getIChefURL(input)).toEqual(expectedOutput);
});

it('builds standard ichef img url based on originCode, locator, resolution and isWebP passed', () => {
const input = {
originCode: 'amz',
locator:
'worldservice/live/assets/images/2013/08/19/130819164754_ardeshir_zahedi_112x63_bbc_nocredit.jpg',
resolution: '660',
isWebP: true,
};
describe('builds WebP ichef img url based on originCode, locator and resolution passed', () => {
const BASE_IMAGE_URL = 'https://ichef.bbci.co.uk';

const expectedOutput =
'https://ichef.bbci.co.uk/ace/ws/660/amz/worldservice/live/assets/images/2013/08/19/130819164754_ardeshir_zahedi_112x63_bbc_nocredit.jpg';
it.each`
originCode | locator | expectedURL
${'cpsprodpb'} | ${'cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.jpg'} | ${`${BASE_IMAGE_URL}/ace/ws/660/cpsprodpb/cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.jpg.webp`}
${'cpsprodpb'} | ${'cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.png'} | ${`${BASE_IMAGE_URL}/ace/ws/660/cpsprodpb/cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.png.webp`}
${'amz'} | ${'worldservice/live/assets/images/2013/08/19/130819164754_ardeshir_zahedi_112x63_bbc_nocredit.jpg'} | ${`${BASE_IMAGE_URL}/ace/ws/660/amz/worldservice/live/assets/images/2013/08/19/130819164754_ardeshir_zahedi_112x63_bbc_nocredit.jpg.webp`}
${'amz'} | ${'worldservice/live/assets/images/2015/05/08/150508054332_cameron_624x351_afp.png'} | ${`${BASE_IMAGE_URL}/ace/ws/660/amz/worldservice/live/assets/images/2015/05/08/150508054332_cameron_624x351_afp.png.webp`}
`(
`for $originCode the expected URL is $expectedURL`,
({ originCode, locator, expectedURL }) => {
const input = {
originCode,
locator,
resolution: '660',
};

expect(getIChefURL(input)).toEqual(expectedOutput);
expect(getIChefURL(input)).toEqual(expectedURL);
},
);
});

it('builds standard ichef img url with originCode mpv', () => {
Expand Down
116 changes: 74 additions & 42 deletions src/sw.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,57 +28,89 @@ describe('Service Worker', () => {
const TEST_IMAGE_URL = 'https://ichef.test.bbci.co.uk';
const BASE_IMAGE_URL = 'https://ichef.bbci.co.uk';

// .webp is removed from the url when the url contains .web already and webp is not supported
describe('image requested in sw when url ends in webp and webp not supported', () => {
describe('image extension (.webp) is stripped and the fallback image is requested when webp not supported', () => {
it.each`
image | expectedUrl
${`${TEST_IMAGE_URL}/news/puppies.jpg.webp`} | ${`${TEST_IMAGE_URL}/news/puppies.jpg`}
${`${BASE_IMAGE_URL}/news/puppies.jpg.webp`} | ${`${BASE_IMAGE_URL}/news/puppies.jpg`}
${`${TEST_IMAGE_URL}/news/puppies.png.webp`} | ${`${TEST_IMAGE_URL}/news/puppies.png`}
${`${BASE_IMAGE_URL}/news/puppies.png.webp`} | ${`${BASE_IMAGE_URL}/news/puppies.png`}
`(`for $image is $expectedUrl`, async ({ image, expectedUrl }) => {
({ fetchEventHandler } = await import('./service-worker-test'));

const event = {
request: new Request(image),
};

event.respondWith = jest.fn();

await fetchEventHandler(event);

expect(event.respondWith).toHaveBeenCalled();
expect(fetchSpy).toHaveBeenCalledWith(expectedUrl, { mode: 'no-cors' });
});
});
image | expectedUrl
${`${TEST_IMAGE_URL}/news/puppies.jpg.webp`} | ${`${TEST_IMAGE_URL}/news/puppies.jpg`}
${`${TEST_IMAGE_URL}/news/puppies.png.webp`} | ${`${TEST_IMAGE_URL}/news/puppies.png`}
${`${TEST_IMAGE_URL}/ace/standard/puppies.jpg.webp`} | ${`${TEST_IMAGE_URL}/ace/standard/puppies.jpg`}
${`${TEST_IMAGE_URL}/ace/standard/puppies.png.webp`} | ${`${TEST_IMAGE_URL}/ace/standard/puppies.png`}
${`${TEST_IMAGE_URL}/ace/standard/assets/images/2015/01/08/150108141819_puppies.jpg.webp`} | ${`${TEST_IMAGE_URL}/ace/standard/assets/images/2015/01/08/150108141819_puppies.jpg`}
${`${TEST_IMAGE_URL}/ace/standard/assets/images/2015/01/08/150108141819_puppies.png.webp`} | ${`${TEST_IMAGE_URL}/ace/standard/assets/images/2015/01/08/150108141819_puppies.png`}
${`${TEST_IMAGE_URL}/ace/ws/puppies.jpg.webp`} | ${`${TEST_IMAGE_URL}/ace/ws/puppies.jpg`}
${`${TEST_IMAGE_URL}/ace/ws/puppies.png.webp`} | ${`${TEST_IMAGE_URL}/ace/ws/puppies.png`}
${`${TEST_IMAGE_URL}/news/assets/images/2015/01/08/150108141819_puppies.jpg.webp`} | ${`${TEST_IMAGE_URL}/news/assets/images/2015/01/08/150108141819_puppies.jpg`}
${`${TEST_IMAGE_URL}/news/assets/images/2015/01/08/150108141819_puppies.png.webp`} | ${`${TEST_IMAGE_URL}/news/assets/images/2015/01/08/150108141819_puppies.png`}
${`${BASE_IMAGE_URL}/news/puppies.jpg.webp`} | ${`${BASE_IMAGE_URL}/news/puppies.jpg`}
${`${BASE_IMAGE_URL}/news/puppies.png.webp`} | ${`${BASE_IMAGE_URL}/news/puppies.png`}
${`${BASE_IMAGE_URL}/ace/standard/puppies.jpg.webp`} | ${`${BASE_IMAGE_URL}/ace/standard/puppies.jpg`}
${`${BASE_IMAGE_URL}/ace/standard/puppies.png.webp`} | ${`${BASE_IMAGE_URL}/ace/standard/puppies.png`}
${`${BASE_IMAGE_URL}/ace/standard/assets/images/2015/01/08/150108141819_puppies.jpg.webp`} | ${`${BASE_IMAGE_URL}/ace/standard/assets/images/2015/01/08/150108141819_puppies.jpg`}
${`${BASE_IMAGE_URL}/ace/standard/assets/images/2015/01/08/150108141819_puppies.png.webp`} | ${`${BASE_IMAGE_URL}/ace/standard/assets/images/2015/01/08/150108141819_puppies.png`}
${`${BASE_IMAGE_URL}/ace/ws/puppies.jpg.webp`} | ${`${BASE_IMAGE_URL}/ace/ws/puppies.jpg`}
${`${BASE_IMAGE_URL}/ace/ws/puppies.png.webp`} | ${`${BASE_IMAGE_URL}/ace/ws/puppies.png`}
${`${BASE_IMAGE_URL}/news/assets/images/2015/01/08/150108141819_puppies.jpg.webp`} | ${`${BASE_IMAGE_URL}/news/assets/images/2015/01/08/150108141819_puppies.jpg`}
${`${BASE_IMAGE_URL}/news/assets/images/2015/01/08/150108141819_puppies.png.webp`} | ${`${BASE_IMAGE_URL}/news/assets/images/2015/01/08/150108141819_puppies.png`}
`(
`for $image the expected fallback is $expectedUrl`,
async ({ image, expectedUrl }) => {
({ fetchEventHandler } = await import('./service-worker-test'));

describe('image not requested in sw when url ends in webp and webp supported', () => {
it.each`
image | headers
${`${TEST_IMAGE_URL}/news/puppies.jpg.webp`} | ${{ accept: 'webp' }}
${`${BASE_IMAGE_URL}/news/puppies.png.webp`} | ${{ accept: 'webp' }}
`(`for $image is $expectedUrl`, async ({ image, headers }) => {
({ fetchEventHandler } = await import('./service-worker-test'));
const event = {
request: new Request(image),
};

const event = {
request: new Request(image, { headers }),
};
// , { headers: { accept: 'jpg' } })
event.respondWith = jest.fn();
event.respondWith = jest.fn();

await fetchEventHandler(event);
await fetchEventHandler(event);

expect(event.respondWith).not.toHaveBeenCalled();
});
expect(event.respondWith).toHaveBeenCalled();
expect(fetchSpy).toHaveBeenCalledWith(expectedUrl, {
mode: 'no-cors',
});
},
);
});

describe('image is not requested in sw', () => {
it.each`
image | headers | reason
${`${TEST_IMAGE_URL}/sport/puppies.jpg`} | ${{}} | ${'image url must end with webp'}
${`${TEST_IMAGE_URL}/ace/standard/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${'webp supported in request headers'}
${`${BASE_IMAGE_URL}/sport/puppies.jpg`} | ${{}} | ${'image url must end with webp'}
${`${BASE_IMAGE_URL}/ace/standard/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${`webp not supported in request headers`}
image | headers | reason
${`${TEST_IMAGE_URL}/sport/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${'image url must include news, ace/standard or ace/ws'}
${`${TEST_IMAGE_URL}/ace/stndard/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${'image url must include news, ace/standard or ace/ws'}
${`${TEST_IMAGE_URL}/ace/sw/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${'image url must include news, ace/standard or ace/ws'}
${`${TEST_IMAGE_URL}/news/puppies.jpeg.webp`} | ${{ accept: 'webp' }} | ${'image extension must be jpg or png'}
${`${TEST_IMAGE_URL}/ace/standard/puppies.jpeg.webp`} | ${{ accept: 'webp' }} | ${'image extension must be jpg or png'}
${`${TEST_IMAGE_URL}/ace/ws/puppies.jpeg.webp`} | ${{ accept: 'webp' }} | ${'image extension must be jpg or png'}
${`${TEST_IMAGE_URL}/news/puppies.jpg`} | ${{}} | ${'image url must end with webp'}
${`${TEST_IMAGE_URL}/news/puppies.png`} | ${{}} | ${'image url must end with webp'}
${`${TEST_IMAGE_URL}/ace/standard/puppies.jpg`} | ${{}} | ${'image url must end with webp'}
${`${TEST_IMAGE_URL}/ace/standard/puppies.png`} | ${{}} | ${'image url must end with webp'}
${`${TEST_IMAGE_URL}/ace/ws/puppies.jpg`} | ${{}} | ${'image url must end with webp'}
${`${TEST_IMAGE_URL}/ace/ws/puppies.png`} | ${{}} | ${'image url must end with webp'}
${`${TEST_IMAGE_URL}/news/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${'webp supported in request headers'}
${`${TEST_IMAGE_URL}/news/puppies.png.webp`} | ${{ accept: 'webp' }} | ${'webp supported in request headers'}
${`${TEST_IMAGE_URL}/ace/standard/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${'webp supported in request headers'}
${`${TEST_IMAGE_URL}/ace/standard/puppies.png.webp`} | ${{ accept: 'webp' }} | ${'webp supported in request headers'}
${`${TEST_IMAGE_URL}/ace/ws/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${'webp supported in request headers'}
${`${TEST_IMAGE_URL}/ace/ws/puppies.png.webp`} | ${{ accept: 'webp' }} | ${'webp supported in request headers'}
${`${BASE_IMAGE_URL}/sport/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${'image url must include news, ace/standard or ace/ws'}
${`${BASE_IMAGE_URL}/ace/stndard/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${'image url must include news, ace/standard or ace/ws'}
${`${BASE_IMAGE_URL}/ace/sw/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${'image url must include news, ace/standard or ace/ws'}
${`${BASE_IMAGE_URL}/news/puppies.jpeg.webp`} | ${{ accept: 'webp' }} | ${'image extension must be jpg or png'}
${`${BASE_IMAGE_URL}/ace/standard/puppies.jpeg.webp`} | ${{ accept: 'webp' }} | ${'image extension must be jpg or png'}
${`${BASE_IMAGE_URL}/ace/ws/puppies.jpeg.webp`} | ${{ accept: 'webp' }} | ${'image extension must be jpg or png'}
${`${BASE_IMAGE_URL}/news/puppies.jpg`} | ${{}} | ${'image url must end with webp'}
${`${BASE_IMAGE_URL}/news/puppies.png`} | ${{}} | ${'image url must end with webp'}
${`${BASE_IMAGE_URL}/ace/standard/puppies.jpg`} | ${{}} | ${'image url must end with webp'}
${`${BASE_IMAGE_URL}/ace/standard/puppies.png`} | ${{}} | ${'image url must end with webp'}
${`${BASE_IMAGE_URL}/ace/ws/puppies.jpg`} | ${{}} | ${'image url must end with webp'}
${`${BASE_IMAGE_URL}/ace/ws/puppies.png`} | ${{}} | ${'image url must end with webp'}
${`${BASE_IMAGE_URL}/news/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${'webp supported in request headers'}
${`${BASE_IMAGE_URL}/news/puppies.png.webp`} | ${{ accept: 'webp' }} | ${'webp supported in request headers'}
${`${BASE_IMAGE_URL}/ace/standard/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${'webp supported in request headers'}
${`${BASE_IMAGE_URL}/ace/standard/puppies.png.webp`} | ${{ accept: 'webp' }} | ${'webp supported in request headers'}
${`${BASE_IMAGE_URL}/ace/ws/puppies.jpg.webp`} | ${{ accept: 'webp' }} | ${'webp supported in request headers'}
${`${BASE_IMAGE_URL}/ace/ws/puppies.png.webp`} | ${{ accept: 'webp' }} | ${'webp supported in request headers'}
`(`for $image because $reason`, async ({ image, headers }) => {
({ fetchEventHandler } = await import('./service-worker-test'));

Expand Down
Loading