Fix case where checked bundles in sidebar don't update #317
Conversation
| @@ -33,6 +33,8 @@ export default class CheckboxList extends PureComponent { | |||
| this.setState({checkedItems}); | |||
| this.informAboutChange(checkedItems); | |||
| } | |||
| } else if (newProps.checkedItems !== this.props.checkedItems) { | |||
There was a problem hiding this comment.
Though I lean towards this being a good short-term fix, this method is now getting difficult to reason about IMO (in way that lines up with some of the arguments made here). A few possibilities to simplify a bit, going from slight change to more radical change:
- Switch from
componentWillReceiveProps(deprecated) togetDerivedStateFromProps. Would make intent clearer IMO, but might not be able to fully replace thethis.informAboutChangepart. - Centralize the "all checked" logic in MobX store
- Use Hooks (could clean up a lot but biggest change / introduces new paradigm to the app)
(I like idea of trying option 2 personally)
There was a problem hiding this comment.
I'll let @th0r weigh in on the refactoring opportunity
For now, as this change fixes the bug, let's ship this and consider the refactor in its own PR as it makes it easier to review a refactor when it keeps the existing functionality 1:1 to what was before.
There was a problem hiding this comment.
Thanks @gaokun and @bregenspan
|
Released in v3.5.2 🎉 |
Fixes #316
The relevant piece of state here is derived from props, and was failing to update upon prop update in the particular case noted in #316.
One thought re: preventing this class of bug in future is to centralize the state management logic that handles whether all bundles are showing, putting it in the shared MobX store (and eliminating use of state derived from props). I might have time to explore this in a separate PR, but I think this quick fix is probably better for now.
Notes re: manual testing
Because the
componentWillReceivePropshandler appears to have been added forwebpack --watchsupport, I tested that the watch mode/dev server compatibility still functions as expected after this change by adding and removing bundles to a running webpack-dev-server. The checkbox state behaved as expected: