-
Notifications
You must be signed in to change notification settings - Fork 840
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[EuiIcon] Early setState yields a console error #4910
Conversation
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? |
There was a problem hiding this 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
const { type } = this.props; | ||
const initialIcon = getInitialIcon(type); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@@ -20,6 +20,7 @@ | |||
- 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)) |
There was a problem hiding this comment.
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.
6a13a4a
to
06a9e00
Compare
06a9e00
to
166f8d8
Compare
166f8d8
to
aacc737
Compare
@thompsongl Changes should be good for re-review :) |
Thanks, @Phizzard Using the reproduction steps mentioned in #4783 (comment) |
@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! |
Why this issue is still not fixed? |
Just doing some cleanups, @Phizzard are you still interested and available to work through this issue? |
Summary
Fixes #4783
Using
<EuiIcon />
with React.StrictMode would yield the following react warningWarning: 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 callsthis.setState()
and is used in the constructor.Moving the the
loadIconComponent()
and logic around it in the constructor tocomponentDidMount()
resolves this warning.Checklist
- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Added or updated jest tests