Esri / calcite-design-system

A monorepo containing the packages for Esri's Calcite Design System
https://developers.arcgis.com/calcite-design-system/
Other
286 stars 76 forks source link

ResizeObserver callback should be wrapped in a requestAnimationFrame #6242

Closed juliannemarik closed 5 months ago

juliannemarik commented 1 year ago

Description

Chrome will occasionally throw an error, ResizeObserver loop limit exceeded. The error is benign and simply means that ResizeObserver was not able to deliver all observations within a single animation frame. You can avoid this error by wrapping the callback in requestAnimationFrame which is what I'm proposing you do as an added precaution in your createObserver function:

// src/utils/observers.ts

...
export function createObserver<T extends ObserverType>(
  type: T,
  callback: ObserverCallbackType<T>,
  options?: ObserverOptions<T>
): ObserverInstanceType<T> | undefined {
  if (!Build.isBrowser) {
    return undefined;
  }

  /**
  * This is the proposed addition: 
  * wrap the resize callback in requestAnimationFrame to prevent a
  * (benign) error related to the observer not being able to deliver
  * all observations within a single animation frame
  */
  let _callback = callback;
  if(type === 'resize') {
   _callback = () => {
      requestAnimationFrame(_ => {
          callback;
      });
    }
  }

  const Observer = getObserver<T>(type);
  return new Observer(_callback as any, options as any) as any;
}

Proposed Advantages

Although the error is benign, it can cause CI to fail, and the proposal is a simple fix which will guard against this possibility

Which Component

observers.ts util

Relevant Info

jcfranco commented 1 year ago

@juliannemarik Thanks for creating this issue and sharing a solution. In terms of severity, how often have you run into the CI failing due to the benign error?

juliannemarik commented 1 year ago

@jcfranco We recently ran into this issue when trying to use the max-items prop on the calcite-dropdown which I believe triggers the resizeObserver callback to determine the height of the dropdown. Using this prop causes our CI (which runs our tests in Chrome) to fail.

This is the first time we've run into the issue, but I imagine we'll run into this issue again if we use other components or component props that leverage the resizeObserver callback.

Ultimately this isn't very high severity, but the Hub team would like to be able to use the max-items prop on the calcite-dropdown, but this error is currently preventing us from doing so.

hccampos commented 1 year ago

Just saw this on our CI as well when trying to add a test for a calcite-dropdown.

jcfranco commented 6 months ago

@juliannemarik @hccampos Can you confirm if this error still occurring with calcite-dropdown (and possibly other components) in the latest release? If so, we could look into the implementation to see if we can mitigate this.

I hesitate to use the suggested workaround for the reasons outlined in this reply. Also, there's a, stalled, discussion regarding this error being changed to a warning instead since it's benign.

hcampos-professional commented 5 months ago

Just tried it again in our CI and it seems like it's working well 👍

jcfranco commented 5 months ago

@hccampos Thanks for testing! Will close based on ☝️.

@juliannemarik Please feel free to reopen if you notice this issue again. Thanks!