Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add React performance warnings #239

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

atcastle
Copy link

@atcastle atcastle commented Nov 3, 2024

Note: This PR is currently a draft and untested, because of a build issue that currently prevents tests from running in the repo. @jerzy-mankowski has said they will address it when they have time. I will revise this PR afterward with a set of passing E2E tests at that time.

This PR includes a draft of 2 new performance warnings for the React AdvancedImage element. Please see the accompanying issue for a full description of the rationale for these two warnings.

The warnings are:
warnOnOversizedImage: Logs a warning when an image is loaded, and the intrinsic size is greater than the rendered size by a substantial margin. That margin is defined in OVERSIZE_IMAGE_TOLERANCE and initially set to 500.
warnOnLazyLCP: Logs a warning when the PerformanceObserver API detects an image is a candidate for the LCP image but has been loaded with loading=“lazy”.

Both warnings only fire if the NODE_ENV variable is present (true in most React apps) and set to “development”. Both warnings can be silenced if the AdvancedImage element is given an attribute called “silence-warnings”.

@jerzy-mankowski
Copy link
Contributor

jerzy-mankowski commented Nov 12, 2024

@atcastle the build issue has been resolved 👍 just need to fix the typing issues

Copy link

@lishaduck lishaduck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happened to notice some typos while vendoring the Cloudinary SDK.

`and may slow down page load. You may address this by supplying a height and width for the image, ` +
`or by using the responsive image plugin. ` +
`Rendered size: ${clientWidth}x${clientHeight}. Intrinsic size: ${naturalWidth}x${naturalHeight}. ` +
`This warning can be surpressed by adding the 'silence-warnings' attribute to AdvancedImage.`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`This warning can be surpressed by adding the 'silence-warnings' attribute to AdvancedImage.`
`This warning can be suppressed by adding the 'silence-warnings' attribute to AdvancedImage.`

`An image with URL ${imgRef.src} has ' loading="lazy"' and has also been detected to be a possible ` +
`LCP element (https://web.dev/lcp). This can have a significant negative impact on page loading performance. ` +
`To fix this issue, remove, 'loading="lazy"' from images which may render in the initial viewport. ` +
`This warning can be surpressed by adding the 'silence-warnings' attribute to AdvancedImage.`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

Suggested change
`This warning can be surpressed by adding the 'silence-warnings' attribute to AdvancedImage.`
`This warning can be suppressed by adding the 'silence-warnings' attribute to AdvancedImage.`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants