[Eslint-plugin] Add require-media-dimensions rule

[whizkydee]

Description

There's a ton of unnecessary layout shifts that happen in the admin and other frontend apps across the org, a lot of which happen in bursts (when one or more individual layout shifts occur in rapid succession with less than 1-second in between each shift and a maximum of 5 seconds for the total window duration — see linked video). A common culprit for this is not specifying explicit dimension attributes for media elements.

Rendering media elements like img and video in the browser without explicitly specifying width and height attributes often would cause an unexpected page movement/jump during load which could interrupt a user's current flow and potentially result in unintended actions which in some cases could be costly — this scenario is called a layout shift.

However, when width and height attributes are specified upfront, modern browsers are able to compute an aspect ratio and reserve the right amount of space while the media loads, leaving only a paint task once the media is ready.

In this video (slowed for illustrative purposes), you'll notice that the layout shifts significantly 4 times in about 2 seconds, which is a lot.

This rule is simply one step towards the right direction of minimizing these jumps — it enforces the presence of width and height attributes on standard media elements (img, video), the Polaris Image component, and provides an avenue to specify custom components and elements that should be analyzed for violations.

There's still a lot that needs to be done to be able to really contain these shifts like exposing interfaces to specify media dimensions for compound components and components that render other media components like EmptyState, Thumbnail etc.

Type of change

• Patch: Bug (non-breaking change which fixes an issue)
• @shopify/eslint-plugin Minor: New feature (non-breaking change which adds functionality)
• Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish a new version for these changes)

[whizkydee]

Thoughts on embedding this in the library itself as opposed to specifying it via the mediaElements option on the consumer-end?

One advantage of the current approach is that we're able to analyze the import specifier and look at the source to ensure that it's actually from @shopify/polaris and not an Image component defined somewhere else which I imagine could cause problems for consumers.

[whizkydee]

When is a good time to turn this on?

[dahukish]

https://shopify.slack.com/archives/C02E6ML0GS1/p1645735515794759?thread_ts=1645721034.727849&cid=C02E6ML0GS1

[ryanwilsonperkin]

How will this handle scenarios where props are not explicitly passed to the component, such as:

const imgProps = {width: 600, height: 400};

return <img {...imgProps} />

There may be cases where we want to conditionally change what the values are set to that would make this approach useful, but it doesn't seem like the current linting setup would handle them. If this is a known limitation of the plugin that's okay but we should document it.

[whizkydee]

Good catch! We can actually account for this, I'll work on a patch.

[whizkydee]

Hm, probably best to not go down the rabbit hole of trying to account for all the possible ways the props can be dynamically passed to the component.

I wonder if we can work around an expected convention though like:

"the spread identifier must be a simple variable defined within the scope of the component for the lint rule to recognize its presence".

This'll make it easier to lookup and provide a site for these conditional props to be stored, otherwise the AST can get really complex when one starts to consider expressions like ...someObject.someOtherObject.imgProps, ...getImgProps(), ...getObject().someObject.imgProps etc.

What are your thoughts on having such constraint around dynamic prop assignment vs not supporting it at all?

[ryanwilsonperkin]

the spread identifier must be a simple variable defined within the scope of the component

So long as this scope includes simple variables that have been imported that should be fine. For example, we want to be able to handle this use-case:

// image-props.js
export default { height: 100, width: 200 }

// component.js
import imageProps from './image-props';

function Component() {
  return <img {..imageProps} />

That likely captures enough of our use case that it'd be acceptable, however it'd still be worth publishing a beta release with this new rule and testing it out in shopify/web to see what the impact is.

[BPScott]

Closing this. I don't want this to become a dumping ground for anybody in the org. If a team wants a new eslint rule then they can maintain their own package rather than trying to shoehorn everything into @shopify/eslint-plugin and then our team having to maintain it forevermore.