fix(EuiSelectableList): fix read-after-write on component state #7768
Conversation
| @@ -349,7 +348,9 @@ export class EuiSelectableList<T> extends Component< | |||
| prevProps.onFocusBadge !== onFocusBadge || | |||
| prevProps.searchable !== searchable | |||
| ) { | |||
| this.forceVirtualizedListRowRerender(); | |||
| this.setState({ | |||
There was a problem hiding this comment.
I didn't want to add an optional optionArray argument to forceVirtualizedListRowRerender, and it felt like using setState directly was the cleanest solution. Let me know if you have other ideas!
There was a problem hiding this comment.
Thanks for catching this! 🙏
Imho, I'd think reusing forceVirtualizedListRowRerender with passing the optionArray might be better for a) comprehension (naming stating what's happening) and b) reusing available code.
But I'm not hard set on this, I'm fine with either.
|
|
||
| // ensure that ListRow updates based on item props | ||
| if (isVirtualized) { | ||
| } else if (isVirtualized) { |
There was a problem hiding this comment.
else if because it's not needed to execute both setStates when the first condition is true
| @@ -349,7 +348,9 @@ export class EuiSelectableList<T> extends Component< | |||
| prevProps.onFocusBadge !== onFocusBadge || | |||
| prevProps.searchable !== searchable | |||
| ) { | |||
| this.forceVirtualizedListRowRerender(); | |||
| this.setState({ | |||
There was a problem hiding this comment.
Thanks for catching this! 🙏
Imho, I'd think reusing forceVirtualizedListRowRerender with passing the optionArray might be better for a) comprehension (naming stating what's happening) and b) reusing available code.
But I'm not hard set on this, I'm fine with either.
cee-chen
added
skip-changelog
tech debt
testing
|
Preview staging links for this PR:
|
💚 Build Succeeded
History
cc @tkajtoch |
Summary
This PR fixes a recently introduced bug (in #7728) in the
EuiSelectableListcomponent caused by read-after-write on its state before the state update is applied. More specifically,forceVirtualizedListRowRerenderreadsthis.state.optionArray, and when called fromcomponentDidUpdate, thesetStatea few lines above isn't yet applied.QA
EUI
gh pr checkout 7768yarn && yarn --cwd packages/eui build-packKibana
gh pr checkout 183431@elastic/euiversion to the local build of EUI inpackage.jsonyarn kbn bootstrap --no-validatenode scripts/jest --config x-pack/plugins/global_search_bar/jest.config.js --watch telmetry.test.tsxpasses all testsGeneral checklist
- [ ] A changelog entry exists and is marked appropriately.Skipping a changelog as this is a fix for a change only made for docs/storybook