Skip to content

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kfule
Copy link
Contributor

@kfule kfule commented May 2, 2025

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 uses Promise.finally to handle the return value and also adds the condition event.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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My change requires a documentation update, and I've opened a pull request to update it already:
  • I have read https://mithril.js.org/contributing.html.

@kfule kfule requested a review from a team as a code owner May 2, 2025 11:10
Copy link
Member

@dead-claudia dead-claudia left a 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 () {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

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.

@kfule kfule requested a review from dead-claudia May 2, 2025 23:38
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.

2 participants