Skip to content

[EuiIcon] Early setState yields a console error#4910

Closed
Phizzard wants to merge 3 commits intoelastic:masterfrom
Phizzard:eui-icon-early-setstate
Closed

[EuiIcon] Early setState yields a console error#4910
Phizzard wants to merge 3 commits intoelastic:masterfrom
Phizzard:eui-icon-early-setstate

Conversation

@Phizzard
Copy link
Copy Markdown
Contributor

@Phizzard Phizzard commented Jun 23, 2021 •
edited
Loading

Summary

Fixes #4783

Using <EuiIcon /> with React.StrictMode would yield the following react warning 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.

This would happen because the loadIconComponent() function calls this.setState() and is used in the constructor.

Moving the the loadIconComponent() and logic around it in the constructor to componentDidMount() resolves this warning.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
    - [ ] Props have proper autodocs and playground toggles
    - [ ] Added documentation
  • Checked Code Sandbox works for the any docs examples
    - [ ] Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 23, 2021

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

thompsongl
thompsongl suggested changes Jun 24, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Phizzard!

src/components/icon/icon.tsx Outdated
Comment on lines +608 to +609
const { type } = this.props;
const initialIcon = getInitialIcon(type);
Copy link
Copy Markdown
Contributor

@thompsongl thompsongl Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has already been called in the constructor. Should be able to just use the icon value in state

Copy link
Copy Markdown
Contributor Author

@Phizzard Phizzard Jun 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! I'll make this change

CHANGELOG.md Outdated
- Fixed `pageHeader` display in `EuiPageTemplate` when template is `empty` or `default` ([#4905](https://github.com/elastic/eui/pull/4905))
- Fixed `EuiPageHeader` bottom padding when `borderBottom = true` ([#4905](https://github.com/elastic/eui/pull/4905))
- Fixed incomplete `height` and `width` information in `EuiResizeObserver` ([#4909](https://github.com/elastic/eui/pull/4909))
- Fixed `EuiIcon` from producing console warning in React.StrictMode ([#4910](https://github.com/elastic/eui/pull/4910))
Copy link
Copy Markdown
Contributor

@thompsongl thompsongl Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be sure to merge/rebase master and move this to the correct location; we've had a release(s) and this is now outdated.

@Phizzard Phizzard force-pushed the eui-icon-early-setstate branch from 6a13a4a to 06a9e00 Compare June 30, 2021 03:30
@Phizzard Phizzard force-pushed the eui-icon-early-setstate branch from 06a9e00 to 166f8d8 Compare June 30, 2021 03:32
@Phizzard Phizzard force-pushed the eui-icon-early-setstate branch from 166f8d8 to aacc737 Compare June 30, 2021 03:35
@Phizzard
Copy link
Copy Markdown
Contributor Author

Phizzard commented Jun 30, 2021

@thompsongl Changes should be good for re-review :)

@thompsongl
Copy link
Copy Markdown
Contributor

thompsongl commented Jun 30, 2021 •
edited
Loading

Thanks, @Phizzard

Using the reproduction steps mentioned in #4783 (comment)
I am still getting a "Can't perform a React state update on an unmounted component" warning.
There might be more to this than just moving the initial setState call, but I haven't investigated further.

@Phizzard
Copy link
Copy Markdown
Contributor Author

Phizzard commented Jul 2, 2021 •
edited
Loading

@thompsongl Ah, the way I was reproducing was just wrapping any example of EuiIcon in React.StrictMode. Moving that logic into didMount resolves that, but I can take a little investigation on what causes the warning with the reproduction steps mentioned!

@siminture
Copy link
Copy Markdown

siminture commented Aug 13, 2021

Why this issue is still not fixed?

@chandlerprall
Copy link
Copy Markdown
Contributor

chandlerprall commented Sep 8, 2021

Just doing some cleanups, @Phizzard are you still interested and available to work through this issue?

@cee-chen cee-chen mentioned this pull request May 12, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@thompsongl thompsongl thompsongl requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[EuiIcon] Early setState yields a console error

5 participants

@Phizzard @kibanamachine @thompsongl @siminture @chandlerprall