Skip to content
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

Fix case where checked bundles in sidebar don't update #317

Merged

Conversation

@bregenspan
Copy link
Contributor

@bregenspan bregenspan commented Sep 25, 2019

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 componentWillReceiveProps handler appears to have been added for webpack --watch support, 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:

  • in the case where all checkboxes were checked before the new bundle was added, the new bundle is added in checked state
  • in the case where not all checkboxes were checked before the new bundle was added, the new bundle is not added in checked state
@valscion
Copy link
Member

@valscion valscion commented Sep 25, 2019

@gaokun can you test out this PR to see if it fixes your problem in #316?

@@ -33,6 +33,8 @@ export default class CheckboxList extends PureComponent {
this.setState({checkedItems});
this.informAboutChange(checkedItems);
}
} else if (newProps.checkedItems !== this.props.checkedItems) {

This comment has been minimized.

@bregenspan

bregenspan Sep 25, 2019
Author Contributor

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:

  1. Switch from componentWillReceiveProps (deprecated) to getDerivedStateFromProps. Would make intent clearer IMO, but might not be able to fully replace the this.informAboutChange part.
  2. Centralize the "all checked" logic in MobX store
  3. Use Hooks (could clean up a lot but biggest change / introduces new paradigm to the app)

(I like idea of trying option 2 personally)

This comment has been minimized.

@valscion

valscion Sep 26, 2019
Member

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.

@gaokun
Copy link
Contributor

@gaokun gaokun commented Sep 26, 2019

@gaokun can you test out this PR to see if it fixes your problem in #316?

Tested, it works now! ^_^

Copy link
Member

@valscion valscion left a comment

Thanks @gaokun and @bregenspan ☺️. I'll merge this soon and do a new patch release.

@valscion valscion merged commit c619d51 into webpack-contrib:master Sep 26, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@valscion
Copy link
Member

@valscion valscion commented Sep 26, 2019

Released in v3.5.2 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.