Enhance JS replacement mechanism for WebP to JPEG to more reliably replace full file name #443
Conversation
mukeshpanchal27
added
[Type] Enhancement
mukeshpanchal27
requested review from
adamsilverstein and
felixarntz
as code owners
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@mukeshpanchal27 Left some feedback. Most importantly, we need to rely on the actually available MIME types for each size rather than on the filter value for which sizes should have additional MIME types.
|
|
||
| // If a full image is present in srcset, it should be updated. | ||
| if( srcset && media[i].media_details.sources['image/webp'] ) { | ||
| srcset = srcset.replace( media[i].media_details.sources['image/webp'].file, media[i].media_details.sources['image/jpeg'].file ); |
There was a problem hiding this comment.
nitpick - i believe core JS coding standards still specify spaces around variables in brackets, eg media[ i ]
There was a problem hiding this comment.
Thank you for pointing out these changes. The PR does not validate JS coding standards. Do we need to fix it? Can I raise the issue of tracking it?
The PR was updated with JS coding standards.
There was a problem hiding this comment.
yes, it would be good to add linting for JS if that is missing
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@mukeshpanchal27 Thanks for the updates. The approach here now looks good, however I think there's in fact another underlying problem that makes the implementation here overly complicated: The plugin's implementation of the REST API response integration is inconsistent with the core implementation of the endpoint - we need to also add the sources to the full size entry within media_details.sizes. See details in my comments below.
That is crucial for the core implementation too and should be straightforward, so it would be great if you could make this change in the rest-api.php file as part of this pull request. From there, we can simplify the JS code to no longer have any special treatment for the full size, as it should be handled just like any other size.
| if ( srcset && media_sources['image/webp'] ) { | ||
| srcset = srcset.replace( media_sources['image/webp'].file, media_sources['image/jpeg'].file ); | ||
| } |
There was a problem hiding this comment.
This code makes sense the way our REST API response is currently structured, however I just checked that again and found out our REST API implementation is inconsistent with core's:
- WordPress core returns the data for the
fullimage withinmedia_details.sizesas opposed to a separate field. When you review the WordPress core implementation of the REST endpoint, you will see that it adds thefullimage data tomedia_details.sizes. This is different from how the metadata itself is structured, but we should stick to the convention that core has established here. - So we should probably monitor this in our REST API implementation and only provide the
fullimage'ssourceswithin that value too, instead ofmedia_details.sources. - Let's adjust our implementation in the
rest-api.phpfile so that it also includes the correctsourcesfor thefullimage size.
Once we have made that change, we will no longer need this extra call here. We can simply loop through sizes_keys like below, because that will then include the full size as expected. It should notably simplify the logic here.
There was a problem hiding this comment.
Thank you for the review. This feedback is addressed.
Additional to this I found one case in which the REST API endpoint does not return sizes. If you upload an image with 100X100 size then we don't get sizes in media_details to tackle this case:
- Check if
sourcesare present inmedia_detailsthen we don't havesizesfor that image in that case update imagesrconly and break the loop https://github.com/WordPress/performance/blob/fix/426-enhance-js-replacement-mechanism/modules/images/webp-uploads/fallback.js#L19-L24 - We have unset
sourcesif we have the full size insizeshttps://github.com/WordPress/performance/blob/fix/426-enhance-js-replacement-mechanism/modules/images/webp-uploads/rest-api.php#L49
Please review it and share your feedback for this implementation.
There was a problem hiding this comment.
@mukeshpanchal27 Ah, good point. Sorry, I only read this after my review. I will update my one comment below to reflect this, as I agree with your observation.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@mukeshpanchal27 Thank you for the updates. The REST API logic looks right now, I only have some small comments on improving and simplifying the code there. The JS logic also looks good for the most part, there are just a few quirks left on removing references to the root media_details.sources and the part of assuming those sources apply to src.
adamsilverstein
left a comment
adamsilverstein
left a comment
There was a problem hiding this comment.
left one more small question/point of feedback; this is ready for some more testing!
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@mukeshpanchal27 This is almost good to go, I see two remaining problems here.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@mukeshpanchal27 Excellent work!
Now that this has been completed, we can add the remaining changes to WordPress/wordpress-develop#3034 and finalize that core PR. 🎉
Summary
In this PR I use
media[i].media_details.sizesto check if the image size has an additional mime type or not. I also usewebp_uploads_get_image_sizes_additional_mime_type_support()to get only allowed sizes, so we don't need to check for other sizes.The
console.logfor the image data.Fixes #426
Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.