Fixed onDismissed being called twice when visible changes value #1281
Conversation
| if (this.hidden) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Not a fan of adding a guard for an easy way out... I'd prefer if we can fix the underlying issue, but I wasn't sure what that issue is due to _visible, @visible, hidden seemingly tracking the same thing (I didn't investigate how these are different).
If we do go with adding the guard, I think I'd recommend adding an inline comment to explain its purpose.
There was a problem hiding this comment.
Yeah, I'll also prefer finding the root cause for this... maybe putting a break point into the hide method to see why it get's called twice? I am unsure why that is, given the did-update modifier should call it only once.
There was a problem hiding this comment.
I'm not sure yet why either. Yep, I can look into it further this week.
There was a problem hiding this comment.
I expected that the bug is caused by setting the property bound to @visible to false twice.
In the reproduction given in #1264 the hideAlert method is called twice. First by the event listener attached to the button and afterwards by @onDismissed event. hideAlert method sets showAlert, which is bound to @visible to false even if it's false before. This invalidates _visible property of the component, which triggers {{did-update}} modifier.
A simplified reproduction of that might look like this:
this.visible = true;
await render(hbs`<BsAlert @visible={{this.visible}}>`);
// this is mocking the event listener on button
this.visible = false;
// await at least one runloop cause {{did-update}} modifier is not fired
// multiple times for the same runloop
await new Promise((resolve) => next(resolve));
// this is mocking the action bound to @onDismissed event
this.visible = false;If I recall correctly the bug could not only be reproduced by clicking the button but also with the dismiss button rendered by the component itself. In that case it's a little bit more complex as two different states are changing: clicking the dismiss button rendered by alert component changes the internal state of <BsAlert>, @onDismissed event changes the state passed in from outside (this.showAlert bound to @visible argument). But as in the example before _visible property of <BsAlert> is invalidated twice, which triggers the {{did-update}} modifier twice.
To summarize: I assumed that setting property bound to @visible argument is required to reproduce the bug. I'm a little bit confused cause the tests added doesn't seem to do so. So maybe my assumption is wrong. Haven't verified.
If my assumption is right, adding a guard as you have done here, seems to be the correct fix in my opinion.
There was a problem hiding this comment.
Apologies for my slow progress with this pull request.
// this is mocking the event listener on button
this.visible = false;// await at least one runloop cause {{did-update}} modifier is not fired
// multiple times for the same runloop
await new Promise((resolve) => next(resolve));// this is mocking the action bound to @onDismissed event
this.visible = false;
After the await render() call in my tests, I tried including these lines of code.
The line await new Promise((resolve) => next(resolve)); didn't trigger the this.dismissed function that I passed to the component (i.e. @onDismissed was called 0 times).
If I continued to use await settled() instead, @onDismissed was called 1 time. Not sure yet what to make of this observation.
|
@jelhan @simonihmig I noticed that the canary tests had failed during a scheduled CI. The same canary tests seem to be failing in this pull request. Could you have a look? My current solution to the issue to add a guard to control the state machine. I feel like there can be a better solution, however. What would you suggest that we do? |
| if (this.hidden) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Yeah, I'll also prefer finding the root cause for this... maybe putting a break point into the hide method to see why it get's called twice? I am unsure why that is, given the did-update modifier should call it only once.
| if (this.hidden) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I expected that the bug is caused by setting the property bound to @visible to false twice.
In the reproduction given in #1264 the hideAlert method is called twice. First by the event listener attached to the button and afterwards by @onDismissed event. hideAlert method sets showAlert, which is bound to @visible to false even if it's false before. This invalidates _visible property of the component, which triggers {{did-update}} modifier.
A simplified reproduction of that might look like this:
this.visible = true;
await render(hbs`<BsAlert @visible={{this.visible}}>`);
// this is mocking the event listener on button
this.visible = false;
// await at least one runloop cause {{did-update}} modifier is not fired
// multiple times for the same runloop
await new Promise((resolve) => next(resolve));
// this is mocking the action bound to @onDismissed event
this.visible = false;If I recall correctly the bug could not only be reproduced by clicking the button but also with the dismiss button rendered by the component itself. In that case it's a little bit more complex as two different states are changing: clicking the dismiss button rendered by alert component changes the internal state of <BsAlert>, @onDismissed event changes the state passed in from outside (this.showAlert bound to @visible argument). But as in the example before _visible property of <BsAlert> is invalidated twice, which triggers the {{did-update}} modifier twice.
To summarize: I assumed that setting property bound to @visible argument is required to reproduce the bug. I'm a little bit confused cause the tests added doesn't seem to do so. So maybe my assumption is wrong. Haven't verified.
If my assumption is right, adding a guard as you have done here, seems to be the correct fix in my opinion.
should be fixed by #1284 |
ijlee2
changed the title
There was a problem hiding this comment.
Looks good to me. I have verified that I can not reproduce the bug manually. Additionally it contains two additional tests, which fail on master but not with this fix.
If you still have some time to work on this one @ijlee2 an additional test case that onDismissed is not called when @visible changes from false to false would be awesome. Having onDismissed called every time when visible is dirtied even and it's false afterwards seems to be the root cause for the endless loop I'm seeing in #1264 (comment).
|
@jelhan Thanks for reviewing my code. Yep, I can work on adding an extra test case. |
|
I would like to get this merged asap, to cut a new release. My understanding is that this is basically ready, given that it fixes the original issue, with the some possible "bonus points" for adding that additional test, right? @ijlee2 we can remove the draft status and merge it from your point of view, right? |
|
@simonihmig Yes, you are correct, thanks for confirming with me. Sorry for the delays. 😓 |
|
Will push a commit with the extra test now. |
| hbs`<BsAlert @fade={{false}} @onDismissed={{this.dismissed}} @visible={{this.isAlertShown}}>Test</BsAlert>` | ||
| ); | ||
|
|
||
| this.set('isAlertShown', false); |
There was a problem hiding this comment.
I think this will address the case described in #1281 (review).
Description
This pull request addresses and closes #1264.
How to Review
I recommend reviewing commits one by one. In the 1st commit, I introduced failing tests that should pass once the bug is fixed.