Allow developers to select which image format to use for images in the content

[eugene-manuilov]

Summary

Fixes #187

Relevant technical choices

Checklist

• PR has either [Focus] or Infrastructure label.
• PR has a [Type] label.
• PR has a milestone or the no milestone label.

[mitogh]

Minor comment about the code.

Would be great to introduce some tests to test cases like:

• Default behavior
• Reverse behavior
• Empty value
• Unsupported mime types

[felixarntz]

@eugene-manuilov Looks great, just one comment.

[adamsilverstein]

Overall looks good although I still prefer to have the filter operate in the opposite order. Also, it would be good to add some tests verifying that the filter works as expected.

[felixarntz]

@adamsilverstein

Overall looks good although I still prefer to have the filter operate in the opposite order.

The benefit of how it currently behaves is that the order of how items have to be sorted in the filter itself is arguably more aligned with the other filter, with the "foundational" format coming first (typically JPEG for our purposes currently). The reason we then iterate through the array backwards here is because the "priority order" of the formats would be actually the opposite, which especially makes sense when thinking about it in the context of future picture element support. Basically, the most advanced modern format should come first, but it's also the most likely format to not be supported by the browser. So there have to be fallbacks, all the way back to the "foundational" format which is expected to work everywhere.

Also, it would be good to add some tests verifying that the filter works as expected.

+1 to that, @eugene-manuilov could you add a few tests for the filter usage?

[adamsilverstein]

which especially makes sense when thinking about it in the context of future picture element support.

It actually makes less sense in the picture context where the sources array is stored in the order you want the browser to use the image type. For example, if you want to use AVIF and fall back to WEBP and then JPEG, you would put the AVIF sources array first, the WEBP array second and finally put the jpeg in the contained img element.

The way we have it now, we will need to reverse the order before building the picture element. The fact that we have to reverse the array every time we use it is surely a sign we have the data in the wrong order?

The image formats should be listed in the order in which you prefer to use the formats, not the reverse!

is arguably more aligned with the other filter

I see these as two separate filters - one is a map of arrays and one is an array.

Since we can't seem to agree on this point, can we put it to a vote? I'll accept the majority view.

[felixarntz]

@adamsilverstein I see your point around reversing the array, I agree that is not optimal. At the same time, my personal take is that it makes sense to have the orders for the two filters behave similarly so that it's easier to remember for developers (if we get rid of the array_reverse, it would mean e.g. that the other filter from #227 would need to have JPEG first and this one here would need to have JPEG last).

Since we can't seem to agree on this point, can we put it to a vote? I'll accept the majority view.

Definitely, that may make sense, maybe even with some further discussion. I'm also happy to be outvoted. :) However, I'd suggest we do this in a follow-up issue to not block this PR which would be crucial to get into the next beta release. Would it be okay with you to for now go with the current approach, and open an issue to follow up on the discussion here? We would then put that into the next beta milestone, which would give us some time to discuss and have a vote.

[felixarntz]

Let's follow up on the filter order discussion in #187 (comment), it's easier to follow on the issue than on this PR.

[felixarntz]

Just one tiny recommendation, but nothing blocking.

[eugene-manuilov]

@adamsilverstein @felixarntz an additional test is added. Not sure if we need more tests because the majority of cases are already covered by existing tests.

[felixarntz]

@eugene-manuilov Updated this PR to be against the 1.0.0-beta.2 branch.