-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add optional callback to forceUpdate method #592
Conversation
Awesome, and it's only 7 bytes! |
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.
LGTM :)
Callback is good, but I got to point out that if you are going to make forceUpdate() async someday, then that's major change (e.g. I rely on it to be sync). |
Agreed - I was going to suggest that might be worth bringing up, but I'm definitely relying on it being synchronous as well. I don't think we can reasonably do that in the near term. I'd be curious what the filesize would be with a more naive callback solution since it's always synchronous: forceUpdate(callback) {
renderComponent(this, FORCE_RENDER);
if (callback) callback();
} (not suggesting that's the right way to go here, just crossed my mind) |
@developit yeah, I thought about it too. There is no right way here, both are okay :-) ( |
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.
Thank-you for the contribution 👍 - I have made a couple of suggestions for improving the tests if you have time to address them in a follow-up PR.
forceUpdate(); | ||
|
||
expect(ForceUpdateComponent.prototype.componentWillUpdate).to.have.been.called; | ||
expect(ForceUpdateComponent.prototype.forceUpdate).to.have.been.called; |
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 for adding these tests, although it doesn't actually check that the component got re-rendered. Ideally the test would do something mirroring a simplified real-world use of forceUpdate
. Something like:
- Create a component whose output depends on something other than props/state
- Render
- Change the external state
- Force update
- Check that the rendered output reflects the updated external state.
expect(ForceUpdateComponent.prototype.forceUpdate).to.have.been.called; | ||
}); | ||
|
||
it('should add callback to renderCallbacks', () => { |
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.
renderCallbacks
is an implementation detail which we should avoid in the description. What the test actually does, checking that forceUpdate
invokes its callback, is fine though.
This PR aims to solve #434. I've also added a general
forceUpdate
is called and rerenders the component test since there didn't seem to be one. Let me know if I placed that in the right test file as I was unsure.