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 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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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 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

Choose a reason for hiding this comment

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

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
@valscion
Copy link
Member

Released in v3.5.2 🎉

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

Successfully merging this pull request may close these issues.

[Bug] Sidebar didn't show correct chunks when fixing.
3 participants