Using loader prop causes maximum depth error when sequentially loaded

[Ecksters]

Describe the bug

It appears that when you use the loader prop an error can happen when loading many SVGs one after the other.

The error points out these lines:

  useEffect(() => {
    if (status === STATUS.LOADED) {
      getElement();
    }
  }, [status, getElement]);

And the setState here:

  const getElement = useCallback(() => {
    try {
      const node = getNode({ ...props, handleError, hash: hash.current, content }) as Node;
      const convertedElement = convert(node);

      if (!convertedElement || !isValidElement(convertedElement)) {
        throw new Error('Could not convert the src to a React element');
      }

      setState({
        element: convertedElement,
        status: STATUS.READY,
      });
    } catch (error: any) {
      handleError(error);
    }
  }, [content, handleError, props]);

The error is:

hook.js:608 Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or one of the dependencies changes on every render.
overrideMethod	@	hook.js:608
getRootForUpdatedFiber	@	react-dom-client.development.js:3867
enqueueConcurrentHookUpdate	@	react-dom-client.development.js:3820
dispatchReducerAction	@	react-dom-client.development.js:6914
(anonymous)	@	index.tsx:97
(anonymous)	@	index.tsx:201

Your minimal, reproducible example

https://codesandbox.io/p/devbox/blissful-turing-hh9rg5?workspaceId=ws_N9re2seS9MZVKPnJDFcBRH

Steps to reproduce

Visit Sandbox linked above, open the console, click the Show Icons button in the Preview, error should appear in the console, may need to wait for more of the icons to load.

Expected behavior

No maximum depth errors when using the loader prop.

How often does this bug happen?

Often

Screenshots or Videos

No response

Platform

No response

react-inlinesvg version

4.2.0

TypeScript version

5.8.2

Build tool

Vite

Additional context

No response

[gilbarbara]

Hey @Ecksters
Does this error happen in everyday use? Do you use 1000 icons all at once?
Any ideas to fix? Feel free to open a PR.

[Ecksters]

In our case we have potentially hundreds of nodes that each spin off a network request and so they finish loading at different times, and each has multiple icons on them, when they finish loading they swap out skeleton loaders for the content, including the icons.

It seems like the upper limit it can handle before it errors is lower if there are other elements doing rendering and having state reactions, in our situation we're seeing it happen in the low 20s of nodes, reducing other state updates increased what we could handle to maybe 30-40, but didn't eliminate it entirely as long as the icons were present.

I don't know what a fix for this would look like, I assume something about the timing causes the useEffect and setState to start stacking.

We have a workaround for now that involves just rendering the icons all together and avoiding the staggered loading, so essentially we can't have them behind the skeleton loaders, but it's acceptable.

[Ecksters]

I've looked into this a bit, but of course my understanding of the library's inner workings might not be very good.

I'm noticing that we have two hooks:

  useEffect(() => {
    if (!previousState) {
      return;
    }

    switch (status) {
      case STATUS.LOADING: {
        if (previousState.status !== STATUS.LOADING) {
          getContent();
        }

        break;
      }
      case STATUS.LOADED: {
        if (previousState.status !== STATUS.LOADED) {
          getElement();
        }

        break;
      }
      case STATUS.READY: {
        if (previousState.status !== STATUS.READY) {
          onLoad?.(src, isCached);
        }

        break;
      }
    }
  }, [getContent, getElement, isCached, onLoad, previousState, src, status]);

  // Handles content loading
  useEffect(() => {
    if (status === STATUS.LOADED) {
      getElement();
    }
  }, [status, getElement]);

That both are watching for status changes, and will call getElement if status is STATUS.LOADED. Is the 2nd hook redundant? I haven't tested to see if removing it helps with this issue at all, and I'm uncertain how the previousState factors into all this.

I'm also wondering if it might have something to do with the props in the dependecy array for the getElement callback, which again might trigger both of these to also change if something changed the props.