feat: Redraw once promise returned from event handler is settled #2862
+343
−126
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EtiamNullam
requested review from
dead-claudia and
StephanHoyer
as code owners
dead-claudia
requested changes
Sep 1, 2023
render/render.js
Outdated
| if (this._ && ev.redraw !== false) { | ||
| (0, this._)() | ||
| if (result != null) { | ||
| if (typeof result.finally === "function") result.finally((0, this._)) |
There was a problem hiding this comment.
Let's just use .then. It's redundant to do both that and .finally, because per spec p.finally(f) is roughly just .then(async v => { await f(); return v }, async e => { await f(); throw e }), complete with the .then method call being observable.
render/render.js
Outdated
| (0, this._)() | ||
| if (result != null) { | ||
| if (typeof result.finally === "function") result.finally((0, this._)) | ||
| else if (typeof result.then === "function") result.then((0, this._)) |
There was a problem hiding this comment.
- Your
(0, expr)is redundant. It's not immediately called, so it does nothing. - Let's do it even on error, similarly to my above
finallyto ensure errors get reported correctly. (Obviously, you can't useasync/await, but it's fairly straightforward.)
There was a problem hiding this comment.
- I was not sure about the use of this
(0, expr)so I've tried to keep it consistent. I'm gonna callthis._directly. - Not sure what you mean by "my above
finally" but I guess I can just passthis._to bothresolveandrejectcallbacks ofthen, like this:
if (result != null && typeof result.then === "function") result.then(this._, this._)I just need to update it with the tests as it seems to work as expected. Thank you!
|
Let me know if there's anything more I have to do. I've applied the requested changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In this PR I'm proposing a solution to handle async event handlers.
I've decided to duck-type for
finallywith fallback tothen. I assume we still want to redraw on rejected promise.NOTE: seems like
onbeforeremovelifecycle event handler is also dealing with promises so maybe some common logic can be extracted.Motivation and Context
I think it's reasonable to redraw after asynchronous action is finished (resulting from user action). I was surprised it doesn't work like that and seems like it can!
More details in the issue here: #2861
How Has This Been Tested?
It seems to work according to my manual testing and it doesn't fail any existing tests.
I'm marking it as a draft as I still need to work out how to make tests work for asynchronous code. Some help would be appreciated. I will hopefully include them soon.Solved.Types of changes
Checklist:
docs/changelog.mdEDIT: Updated
docs. I'm not sure how I'm supposed to update changelog, as it seems like it was not updated recently.EDIT2: Added tests. Let me know if they are not sufficient.