[EuiIcon] Early setState yields a console error

[closingin]

Hello guys!

I just noticed that there's a little bug on the EuiIcon component, as a this.setState is called too early in the process, yielding the following console error : Warning: Can't call setState on a component that is not yet mounted. This is a no-op, but it might indicate a bug in your application. Instead, assign to 'this.state' directly or define a 'state = {};' class property with the desired state in the EuiIcon component.

Looking at the codebase, the problem lies in the component constructor on l.594 where loadIconComponent is called, triggering the setState on l.635

I don't have much background in this library, but from what I see you're building an icon cache for every EuiIcon that gets spawned, to improve rendering performance of other EuiIcons with the same iconType? If that is so, I think moving the problematic code to componentDidMount would fix the problem without breaking much things.

I'm sorry that I don't have the time to open a PR 😔

[thompsongl]

Thanks, @closingin
Do you have a reliable way to reproduce the issue? I haven't been able to get a setState error in my local environments.
You're probably right about moving the initial load call to componentDidMount, but I'd like to be able to demonstrate the change in behavior.

[jeanpauldejong]

Thanks for raising @closingin I've just run into this issue with the same warning message and @thompsongl I can reliably reproduce this.

When I use an Form Control EuiFieldText component and I use the prop icon I get the error, when I remove the icon prop the error goes away.

I'm using version 34.2.0, I hope this helps.

[myasonik]

Thanks @jeanpauldejong!

I tried to create a minimal test case in CodeSandbox but I'm not able to reproduce it.

Can you provide a CodeSandbox link with the error? That'll help us resolve this issue quickest.

[jeanpauldejong]

Sure no worries. Here is an example. The SignInForm.tsx has only two fields EuiText with an icon for email and the EuiPassword field. The moment I disable the display of such icons the error goes away. I hope that helps.

https://codesandbox.io/s/elastic-eui-euiicon-i1p56?file=/src/SignInForm.tsx

[myasonik]

Thanks for that!

Using that, I found that the issue is exposed because of React.StrictMode. (Here's the most minimal CodeSandbox I could come up with to recreate the error.)

It's still something we should fix in EUI but, in the mean time, if you turn off strict mode you can avoid the error.

[chandlerprall]

I was able to reproduce in our docs by adding:

  const [icon, setIcon] = useState('accessibility');

  useEffect(() => {
    setTimeout(() => {
      setIcon((icon) => (icon === 'alert' ? 'accessibility' : 'alert'));
    }, 5000);
  });

[...]

    <div>
      <EuiIcon type={icon} />

in src-docs/src/views/button/button.js , and then navigating away from and back to the buttons page.

[Phizzard]

Anyone looking into this? I'd be happy to take it :)

[p-young]

I've also been encountering this bug

[j-m]

Please backport the fix for this to pre v52 (deprecation of legacy theme)

[chandlerprall]

Please backport the fix for this to pre v52 (deprecation of legacy theme)

Our backport strategy excludes this, per https://github.com/elastic/eui/blob/main/wiki/releasing-versions.md

In general, we strongly encourage updating to the latest version of EUI to obtain bug fixes, and we do not actively consider backporting fixes to previous major or minor release versions. The exception to this is when supporting Kibana's release process, as we want to avoid pushing larger changes near the feature freeze.

The legacy theme was deprecated back in September (#5222), and the CSS-in-JS tentative schedule (#3912) has always included some language around deprecating/removing/breaking the legacy theme since it was created.

While the setState warning is annoying (and we're likely getting back to this later in the week), to my knowledge it doesn't affect anything beyond the console log which we would not consider a backport for, either.

[cee-chen]

@thompsongl I'm doing some quick testing on this now and I don't think @chandlerprall's repro steps in #4783 (comment) is valid. The can't perform a React state update on an unmounted component error there would be coming from the contrived setIcon/setState within the setTimeout, not from EuiIcon itself. I think @Phizzard's approach was a perfectly valid fix and we should open a new PR with his fix cherry-picked.

Thoughts?

[thompsongl]

Oh right on. Yeah cherry pick or whatever seems best.