WSTEAMA-749: Update service worker tests and URL generation logic #11595
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,28 @@ | ||
| import { getEnvConfig } from '../getEnvConfig'; | ||
|
|
||
| /* | ||
| The pattern below: | ||
| matches | ||
| https://ichef.test.bbci.co.uk/news/660/cpsprodpb/cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.jpg | ||
| https://ichef.bbci.co.uk/news/660/cpsdevpb/cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.jpg | ||
| https://ichef.test.bbci.co.uk/ace/ws/660/cpsprodpb/cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.jpg | ||
| https://ichef.bbci.co.uk/ace/standard/660/cpsprodpb/cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.png | ||
| https://ichef.test.bbci.co.uk/ace/ws/660/amz/worldservice/cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.jpg | ||
| https://ichef.bbci.co.uk/ace/standard/660/amz/worldservice/cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.png | ||
|
|
||
| does not match | ||
| https://ichef.test.bbci.co.uk/news/660/amz/worldservice/cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.jpg | ||
| https://ichef.bbci.co.uk/news/660/amz/worldservice/cc66/live/5b34d420-b382-11e9-b6fd-e3056fffd1f1.png | ||
| */ | ||
|
|
||
| const webpSupportedPatterns = [ | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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! There was a problem hiding this comment. Brill. |
||
| /^https:\/\/ichef(?:\.test)?\.bbci\.co\.uk\/(?:news|ace\/(?:standard|ws))\/.+(?:\.jpg|\.png)$/, | ||
| /(?:\/ace\/(?:standard|ws)\/.+\/(?:amz\/worldservice\/)?|\/news\/(?!.+\/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'; | ||
|
|
@@ -19,26 +42,27 @@ const buildPlaceholderSrc = (src, resolution) => { | |
| return `https://${newUrl.join('/')}`; | ||
| }; | ||
|
|
||
| const buildIChefURL = ({ | ||
| originCode, | ||
| locator, | ||
| resolution, | ||
| ichefSubdomain = 'ace/ws', | ||
| }) => { | ||
| if (originCode === 'mpv' || originCode === 'pips') { | ||
| return buildPlaceholderSrc(locator, resolution); | ||
| } | ||
|
|
||
| const url = [ | ||
| getEnvConfig().SIMORGH_ICHEF_BASE_URL || 'https://ichef.bbci.co.uk', | ||
| ichefSubdomain, | ||
| resolution, | ||
| originCode, | ||
| locator, | ||
| ] | ||
| .filter(Boolean) | ||
| .join('/'); | ||
|
|
||
| return isSupportedWebpUrl(url) ? `${url}.webp` : url; | ||
| }; | ||
|
|
||
| export default buildIChefURL; | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
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.
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
.webpURLs that we aren't aware of i.e we don't know where the.webpis being set.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.
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.
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.
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
.webpat the end.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.
I've changed this to
(news|images|ace\/(standard|ws))toDo let me know if that's okay.