-
-
Notifications
You must be signed in to change notification settings - Fork 929
feat: Make redraws when Promises returned by event handlers are completed #3020
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
Conversation
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.
I like the basic idea, but not the current execution of it.
Doing redraws always for promises and only on success for normal functions will be too surprising to people when errors are involved.
render/render.js
Outdated
if (eventRedraw && ev.redraw !== false) { | ||
eventRedraw() | ||
if (result != null && typeof result.then === "function") { | ||
Promise.resolve(result).finally(function () { |
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.
Promise.resolve(result).finally(function () { | |
Promise.resolve(result).then(function () { |
I'd rather rejections and thrown errors have the same behavior. It's less surprising to users.
Obviously, this will also require changes to the tests.
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.
OK, I changed finally
to then
and fixed the test.
Note: @dead-claudia @kfule I don't think this should be a semver major change. I've started a thread on zulip here for us to discuss. |
@JAForbes And responded. We can continue the discussion there. |
This PR allows redraw on completion of the async event handler.
Description
This PR makes redraws when Promises returned by event handlers are completed. This allows for async event handlers to be redrawn on completion of the function as well as synchronous event handlers.
Motivation and Context
This is like re-opening the #2862 that was accidentally closed.
Unlike #2862 , this PR
usesadds the conditionPromise.finally
to handle the return value and alsoevent.redraw === false
. Therefore, I did not cherry-pick #2862 .How Has This Been Tested?
I added the tests and confirmed that
npm run test
passes.I also added eslintrc because the new test code includes the async functions.
Types of changes
Checklist