w3c / csswg-drafts

CSS Working Group Editor Drafts
https://drafts.csswg.org/
Other
4.47k stars 659 forks source link

[resize-observer] Should "resize loop error notification" be a warning instead of an error? #5488

Open trusktr opened 4 years ago

trusktr commented 4 years ago

The spec says

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.

which seems like the intent is for this to be a warning, because behavior is not to halt a program, but to advise that remaining resize observations will be fired in the next loop.

In Chrome the message is ResizeObserver loop limit exceeded. Valid error handling code (as a search for this on GitHub shows) gets caught up by this error and programs are unintentionally halted for what seems like a warning.

As an example (at time of writing this), https://lume.io/docs/#/examples/shadow-dom shows that this error can cause unexpected termination of a program (downside the window to trigger termination of the program in on the right of the code box).

Here are some GitHub issues showing users having issues with termination by 3rd libraries due to this "error": https://www.google.com/search?q=site%3Agithub.com%20ResizeObserver%20loop%20limit%20exceeded&cad=h

I think perhaps treating this as a warning might be a better deal; perhaps "resize loop warning notification".

SystemParadox commented 4 years ago

We've started getting this error (and the Firefox equivalent: ResizeObserver loop completed with undelivered notifications.) even though none of our code or any libraries uses ResizeObserver.

The only thing I can think of is that browser extensions are allowed to create ResizeObservers for elements on our page. If this is the case then the current behaviour of throwing an uncaught error to our error handler is utterly broken and needs fixing ASAP.

css-meeting-bot commented 4 years ago

The CSS Working Group just discussed [resize-observer] Should "resize loop error notification" be a warning instead of an error?.

The full IRC log of that discussion <dael> Topic: [resize-observer] Should "resize loop error notification" be a warning instead of an error?
<dael> github: https://github.com/w3c/csswg-drafts/issues/5488
<dael> fantasai: I didn't know what to do but seems like we should do something
<dael> Rossen_: Anyone had a chance to look at this and propose behavior or take a stance on warning vs error?
<dael> iank_: Not sure downgrading completely to warning is correct. I have heard webdev uncover real bugs in production. Requires thought
<dael> iank_: I think this requires a little bit of thought about how to approach this
<dael> Rossen_: Let's continue in the issue then
<dael> Rossen_: Once you're ready tag it again and we'll look
hilts-vaughan commented 3 years ago

Hi.

I've been working with the ResizeObserver for some time now trying to write some web components. I've ran into a few problems using it and I'm going to try and use this time to decompose what those are and where I think there is room for improvement.

It makes perfect sense to report this but probably not as a silent error that cannot be captured by the runtime; this seemingly breaks the model that developers are used to. There are a few problematic reasons for this and I will try and enumerate them here:

  1. You can have extension exceptions that throw this when using ResizeObserver (and many applications of it use it "wrong") and they get reported as "exceptions" on window.onerror but nowhere else. This is an issue because they are reported as a global error even if it's "intentional" -- see the next point. Some of the most popular answers on StackOverflow are to simply ignore them which is a bit of a silly thing to do if they are legitmate -- but you can't discern legitmate ones from not since you can't actually try...catch the exception and handle it yourself (for example, if it's a legitmate one, and you expect it, perhaps you will try...catch it; for more info you can see the next point)

Over and over, you can find comments like this https://github.com/WICG/resize-observer/issues/38#issuecomment-422126006 that state the "error is benign" or the most upvoted StackOverflow answer, https://stackoverflow.com/a/50387233. There are a ton of folks here as well https://github.com/WICG/resize-observer/issues/38#issuecomment-616445073 and many commits mentioning that they are simply filtering it out from Sentry. Or others: https://github.com/WICG/resize-observer/issues/38#issuecomment-422126006

Lately, there are a ton of of these coming through even more recently:

  1. https://github.com/grafana/grafana/issues/31599 ("tracing back to some discussions on the web, it feels like the error shouldn't be an error but a warning.")
  2. https://github.com/alibaba/ali-react-table/issues/150
  3. https://github.com/zooniverse/front-end-monorepo/pull/2047/commits/eb6d2bc7a79d8bb3803ee1c8c2404bb17239f87d
  4. https://github.com/DevExpress/testcafe/issues/4857#issuecomment-598775956
  5. https://github.com/Brightspace/d2l-rubric/pull/692/files
  6. Look @ all the code ignoring it on Github: https://github.com/search?q=ResizeObserver+loop+limit+exceeded&type=code

There are a ton more, check out all the linked things from here: https://github.com/WICG/resize-observer/issues/38

This isn't the hallmark of a good API and it's not what we want to encourage developers to do.

... if the world just decides to ignore the error then it isn't an error at all anymore. To make matters worse, Firefox has a different string so the entire world can't even deny-list a single message. You can see that some folks are simply coming up the "creative" solutions to fix this: https://github.com/DevExpress/testcafe/issues/4857#issuecomment-598775956

Many folks simply ignore it via RAF / setTimeout by queueing a microtask / microtask but this defeats the purpose -- since you don't get synchronous rendering! For example, see https://github.com/petyosi/react-virtuoso/commit/5475a10d873811b056d059c85fa419e764b70181

The tldr is;

  1. Consumers of the API will simply ignore the exception

  2. Consumers of the API sometimes have legitmate reasons to ignore the exception -- since they may expect a single iteration to settle after and no "jank" will appear

  3. Consumers of the API don't have a choice to "catch" the 2nd case, so many will opt to simply swallow the error and go on from there. This is not desirable because as as developer, if I am dropping a lot of Resize Observer notiications, I may want to know about it, or if my users experience it, I want to know about it. But in some cases it really is benign and I have no control over this.

  4. For layouts that perform self change (but are fine with a single depth iteration), this often gets thrown since there is no better alternative. I just pulled the example from MDN and imported it into JSFiddle with a change to show errors in an alert, you can find it here: https://jsfiddle.net/Lxzmh40u/. This is right from MDN more or less -- it is an exmaple of a "bad" layout change -- since in response to the change. There are a ton of examples that do this online now. It is often nice to be able to change yourself. There are many other libraries that deal with this as well. You can find https://github.com/petyosi/react-virtuoso/issues/254 that has a repo for the issue that triggers it every so often when scrolling -- and the reason is similar; because it does some self modification, once.

I think there is a legitmate use case for these solutions when done right -- so I will open a seperate proposal to fix this.

How to fix it?

Chrome has the notion of violations, though I don't know if they are a web standard: https://umaar.com/dev-tips/192-console-violations/. I think something like that, though, is more well suited. This is not captured by things like Sentry, Bugsnag, and the ilk, and it's verbose enough that it would let you determine performance regressions.

Can we talk about this a bit more?