Image Resizer (Fastly) For Body Images #42
Conversation
This module handles image URLs and srcsets. It also transforms Grid-style URLs from CAPI into image resizer URLs.
Migrated the image resizer code to the Asset module
| typeData: AssetTypeData; | ||
| } | ||
|
|
||
| type Url = string; |
There was a problem hiding this comment.
It seems you define a Url type here, yet you're using Javascript's type URL a little bit later. Should we use URL everywhere?
There was a problem hiding this comment.
Is there anywhere that JavaScript's URL is used as a type? I thought I'd only used it as a value, e.g. the URL constructor.
I created this type alias because I wanted the function types to be a bit more semantically helpful, e.g. Url -> number -> Url vs string -> number -> string. However, if you think it may cause confusion I can remove it?
src/utils/Asset.ts
Outdated
|
|
||
| const params = new URLSearchParams({ | ||
| w: width.toString(), | ||
| h: height.toString(), |
There was a problem hiding this comment.
As of now it's probably fine to have width and height use the size returned by CAPI, but the point of the resizer is to be able to give it a different size than the master image.
That way the CAPI articles link to the master image (a usually large and good quality image), yet the client can resize it to something smaller to save on bandwidth.
We may end up copying dotcom and have 4 or 5 versions of the image, each with a different resolution depending on the device breaking point (wrapped in the picture element).
But you probably already know all of that! That can be done later as a separate ticket. Do you know if there's one in the backlog already?
There was a problem hiding this comment.
each with a different resolution depending on the device breaking point
This.
Also, each in turn, with two dpr values for normal and HiDPI devices.
There was a problem hiding this comment.
This should already be doing that. The width and height here refer to a list of different possible image sizes that CAPI provides us with. The toSrcset function grabs the base image URL from the master image, and then generates a series of images of different sizes for use with img srcset.
There was a problem hiding this comment.
The output from this will look something like:
<img... srcset="
https://i.guim.co.uk/img/media/<image_url>/master/6497.jpg?w=6497&h=3898&q=85 6497w,
https://i.guim.co.uk/img/media/<image_url>/master/6497.jpg?w=2000&h=1200&q=85 2000w,
https://i.guim.co.uk/img/media/<image_url>/master/6497.jpg?w=1000&h=600&q=85 1000w,
https://i.guim.co.uk/img/media/<image_url>/master/6497.jpg?w=500&h=300&q=85 500w,
https://i.guim.co.uk/img/media/<image_url>/master/6497.jpg?w=140&h=84&q=85 140w"
>(I've omitted some of the extra parameters for brevity)
There was a problem hiding this comment.
@paperboyo this approach should also work for multiple DPRs. The browser decides which URL to request, based upon what quality and size it knows its screen to be.
AFAIK dotcom take the additional step of switching to a lower-quality, larger-size version for DPR x2 (although not DPR x3), on the basis that it has a slightly better image quality to bandwidth ratio. We may need to look at doing that in the future.
As a side note, I've found this mdn article very useful for navigating the way browsers handle responsive images!
There was a problem hiding this comment.
switching to a lower-quality, larger-size version for DPR x2
Exactly 👏. Cool. Also, dotcom is not using height because the resizer automatically keeps the ratio. Although there is this.
There was a problem hiding this comment.
Also, dotcom is not using height because the resizer automatically keeps the ratio
Ah, interesting. I derived the query string we're passing from the MAPI implementation, which I believe defines it here.
However, I don't know the historical reason for it being implemented that way (@davidfurey @alexduf @webb04?). If you think it would be preferable to drop the height and rely on the width alone I don't think that should cause any problems.
There was a problem hiding this comment.
Not sure, I think having the height predates my hiring here :)
As discussed, let's hardcode the breakpoints rather than extract them from CAPI
There was a problem hiding this comment.
As discussed, let's hardcode the breakpoints rather than extract them from CAPI
👍 updated it to do this. It also allowed me to delete some functions, which is nice =).
|
|
||
| const template = await readTemplate(); | ||
| const key = await getConfigValue<string>("capi.key"); | ||
| const imageSalt = await getConfigValue<string>('apis.img.salt'); |
There was a problem hiding this comment.
Not sure if it would make much difference, but could we do this in parallel?
let [template, key, imageSalt] = await Promise.allSettled([readTemplate(), getConfigValue<string>("capi.key"), getConfigValue<string>('apis.img.salt')]);
There was a problem hiding this comment.
That requires Node 12.9... Promise.all?
There was a problem hiding this comment.
Agreed capi key won't make it to production, so performance not too much of an issue.
Why are you doing this?
CAPI provides asset information for images that appear in the body of articles. We want to use this information to request images from the image-resizer, as discussed in the comments on #19.
Image resizer URLs are built from the path of the original image asset (provided by CAPI), and a series of query parameters including:
The signing code is derived from what MAPI is currently doing. I think that using the Node crypto library for this should be fine, but please let me know if you see any problems with doing things this way.
This is just for body images. Other images, such as the one in the header, will be coming in a later PR.
FYI @paperboyo @akash1810 @philmcmahon
Note: I'm going to look into a better method for passing config values through the call stack.
Changes
Assettype out into a separate module and located the image resizer code there.async/await. This should make it easier to retrieve multiple config values.