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

fix(driver): suppress ResizeObserver warning #20284

Closed
wants to merge 1 commit into from

Conversation

lmiller1990
Copy link
Contributor

User facing changelog

Suppress ResizeObserver loop limit exceeded error.

Additional details

Since the early days, using ResizeObserver in your code often leads to ResizeObserver loop limit exceeded errors in Cypress when using Chrome. It's a Chrome specific error.

It's generally understood as safe to ignore this error, as we recommend multiple times:

My understanding is this error triggers when ResizeObsever#observe is called more than once in a single requestAnimationFrame, which isn't really a fatal error that should cause a test failure.

This reason I added this into Cypress core is it's blocking some of our own CT examples, see this issue. Rather than telling people to "safely ignore it" and copy-paste a snippet, I figure it makes sense to include it, so they don't need to. Instead, we print a console.warn.

It's worth noting our own code base also intentionally ignores this - this spec cannot run due to ResizeObserver loop error, which is why we added the common accepted work-around here.

How has the user experience changed?

Before (with work-around):

image

After:

image

No need for copy-pasting

Cypress.on('uncaught:exception', (err) => !err.message.includes('ResizeObserver loop limit exceeded'))

in your support file.

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?
  • Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Feb 21, 2022

Thanks for taking the time to open a PR!

@lmiller1990
Copy link
Contributor Author

lmiller1990 commented Feb 21, 2022

Follow up on comment from @marktnoonan in #20257 (comment)

I support ignoring this error by default, a lot of devs have needlessly spun wheels on this issue. A couple of questions:
Could we warn in the command log as well as the console, and link to a part of the docs where we explain that this 1 specific error is ignored and why? It might be useful for videos to still show that a thing happened.
I think I'd want to be able to override this default in config at the top level, or possibly even for a particular block. I could see some people not agreeing that it is always safe to ignore, even though we believe it is.

  1. I don't think it's really useful to show this in the command log. The idea of this PR is that the warning is basically meaningless, so we do not want to pollute the command log with the various things Cypress does under the hood. I think the console.warn seems more appropriate.
  2. How would this override API look? I'd say if we think there's a chance someone wants this, we just don't have this feature at all. I can't find any cases where this warning is actually useful, though, or why anyone would opt into it.

Both your comments kind of hint at the current solution: asking people to add a copy-paste code like we have been for years. It satisfies (1) show in the command log and (2) opt out (don't have the snippet). I don't think this is ideal - at least from my understanding, this warning is meaningless in all situations. I couldn't find any examples where this warning was stopping me from missing an infinite loop, or blocking render, etc.

@cypress
Copy link

cypress bot commented Feb 21, 2022



Test summary

19280 0 220 0Flakiness 0


Run details

Project cypress
Status Passed
Commit 87e9357
Started Feb 21, 2022 1:17 AM
Ended Feb 21, 2022 1:29 AM
Duration 11:29 💡
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@tbiethman
Copy link
Contributor

tbiethman commented Feb 21, 2022

After doing more reading about this error, I'm conflicted about our role here.

  1. The error message has changed in the current draft specification. As this is currently in draft and looks to stay that way for a while, we might find ourselves maintaining this more than we want to. While the message handled in this PR is Chrome specific, Firefox and Webkit currently implement the message in the draft:

ResizeObserver loop completed with undelivered notifications.

  1. Even though it is "safe to ignore", the error is being logged in the first place because the ResizeObserver itself is preventing an infinite loop.

Infinite loop is prevented by shrinking the set of nodes that can notify at every iteration. In each iteration, only nodes deeper than the shallowest node in previous iteration can notify.

An error is generated if notification loop completes, and there are undelivered notifications. Elements with undelivered notifications will be considered for delivery in the next loop.

So while it does course correct, it seems like this is still a correctable issue, in that something must be causing the loop to occur by changing/mutating a higher node during a lower node's observer handler. I'm curious if the linked spec in the description doesn't have an actual issue that could be corrected.

I think the years of "it's not a real problem" has created a bit of a hot potato where we have lots of "problematic" implementations in open source libs today and it can be hard to track down the true causes. The fact that everything seems to work out post-error means that it's just been easier to ignore. Which to be fair I think is a reasonable approach, given the minimal impact, but not necessarily one I think we want to assume for everyone. But the question here is whether it makes sense to show up in the command log at all. And I want to say no, it doesn't, but at the same time it's hard for me to recommend handling this exception differently than any other.

Would it make sense to allow for the suppression of this through config? This would tacitly say "Hey, we realize that the ResizeObserver throws some errors that our users tend to ignore, we can make ignoring them easier for you if you choose to enable this". I'm not a big fan of adding config for everything, either, but this is a fairly unique scenario.

@lmiller1990
Copy link
Contributor Author

lmiller1990 commented Feb 21, 2022

@tbiethman Good points and description. Basically it boils down to:

  1. Is it really safe to dismiss and ignore this on behalf of all users? Hard to say. I cannot create an example where ignoring this exception reveals a bug, though.
  2. Allow for suppression through config? This is similar to having a snippet you can copy-paste, but a bit cleaner, since we'd also remove it from the command log.

How would this look as a config option?

export default defineConfig({
  suppressExceptions: ['ResizeObserver']
})

Or something like this? Would it be opt-in or opt-out? We'd display a console.warn in the case of an opt-out warning getting suppressed, so users know it's happening.

My main concern is, as described here some libraries (like Ant Design's Autocomplete) are basically dead-on-arrival when running in the Cypress AUT without a work-around, unless suppressing this error is the default.


One important thing to note: our SelectorPlayground and Ant's Autocomplete do not have this problem except when rendered in the AUT. Something about getting rendered in the AUT changes something and leads to this error manifesting. This happened in both 9.x and 10.x, so it seems it's something more fundamental than just the Cypress UI, but I don't know what it is. So this PR is mainly suppressing an error that is Cypress specific.

@flotwig
Copy link
Contributor

flotwig commented Feb 23, 2022

Is it really safe to dismiss and ignore this on behalf of all users? Hard to say. I cannot create an example where ignoring this exception reveals a bug, though.

Wouldn't this allow infinite loops from buggy ResizeObservers that themsleves resize elements in their handlers? That seems like what this error is designed to prevent, at least.

Allow for suppression through config? This is similar to having a snippet you can copy-paste, but a bit cleaner, since we'd also remove it from the command log.

IMO we should avoid adding things like this to config. There's already a lot of stuff in there, and for 99.99% of users, they will not want to change this. I'm more in favor of exposing it on Cypress so that users can monkeypatch it out.

For example, you could have a Cypress.Errors.shouldSuppressUnhandled: (err: Error) => boolean function available, and if users really really wanna modify our default unhandled:exception handler, they can monkey-patch that function. This leaves an escape hatch for the 0.01% of users without bloating up the config.

@brian-mann
Copy link
Member

The bigger question and really only question is understanding why resize observer throws in a cypress environment outside a normal environment. Fixing that would obviate the need to change anything.

@jennifer-shehane jennifer-shehane removed the request for review from a team February 23, 2022 19:03
@lmiller1990
Copy link
Contributor Author

We will

  1. investigate why it's happening in Cypress (for the examples that it only happens in Cypress)
  2. how prevalent this is in general, and it component testing

If it turns out this is very common, we can reconsider swallowing the error, but it sounds like the ideal scenario would be understanding why this happens in as @brian-mann suggested. Unless it's very prevalent in CT, we won't add this fix here, but instead recommend a workaround using Cypress.on("unhandledexception").

@freese
Copy link

freese commented Mar 13, 2023

Thanks a lot for the PR and effort @lmiller1990.

I came across the ResizeObserver loop limit exceeded issue, while using on our react project, the content-visibility CSS attribute, to skip rendering work in some cases. This of course combining it with the contain-intrinsic-ratio property.
But I could narrow it down to content-visibility, since after removing it, all tests were passing again. So in case you want to dig into it, you could try this as hint to reproduce the issue and maybe understand better why it's happening.

@lmiller1990 lmiller1990 deleted the UNIFY-940-Resize-Observer-develop branch March 13, 2023 22:21
undergroundwires added a commit to undergroundwires/privacy.sexy that referenced this pull request Apr 3, 2024
This commit addresses false negative failures in Cypress due to a known
Chrome issue.

The included change prevents Cypress tests from failing because of the
non-critical `ResizeObserver loop limit exceeded` error, which occurs
inconsistently during CI/CD runs with GitHub runners. This error has
been documented in CHrome and does not affect actual browser usage or
local test runs. This commit implements a widely recommended workaround
that ignores this specific error during test execution.

Error from Cypress:

```
Error: The following error originated from your application code, not from Cypress.
> ResizeObserver loop limit exceeded
```

The solution follows community-driven advice and past discussions on
handling this benign exception within test scenarios. It contributes to
more reliable CI/CD results by filtering out irrelevant error noise.

For detailed background and discussion on this error, see:

- Cypress issues: cypress-io/cypress#8418, cypress-io/cypress#20341
- Cypress PRs: cypress-io/cypress#20257, cypress-io/cypress#20284
- Discussion in Quasar: quasarframework/quasar#2233
- Discussion in specification repository: WICG/resize-observer#38
undergroundwires added a commit to undergroundwires/privacy.sexy that referenced this pull request Apr 4, 2024
This commit addresses false negative failures in Cypress due to a known
Chrome issue.

The included change prevents Cypress tests from failing because of the
non-critical `ResizeObserver loop limit exceeded` error, which occurs
inconsistently during CI/CD runs with GitHub runners. This error has
been documented in CHrome and does not affect actual browser usage or
local test runs. This commit implements a widely recommended workaround
that ignores this specific error during test execution.

Error from Cypress:

```
Error: The following error originated from your application code, not from Cypress.
> ResizeObserver loop limit exceeded
```

The solution follows community-driven advice and past discussions on
handling this benign exception within test scenarios. It contributes to
more reliable CI/CD results by filtering out irrelevant error noise.

For detailed background and discussion on this error, see:

- Cypress issues: cypress-io/cypress#8418, cypress-io/cypress#20341
- Cypress PRs: cypress-io/cypress#20257, cypress-io/cypress#20284
- Discussion in Quasar: quasarframework/quasar#2233
- Discussion in specification repository: WICG/resize-observer#38
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.

None yet

5 participants