Allow additional async redraw even if the first redraw is skipped #3021

kfule · 2025-05-07T12:56:18Z

Description

This PR allows asynchronous redraw processing even if the first redraw is skipped by setting event.redraw=false before await in the async function.
Also, this PR avoids redraw after the dom or component is removed.

Motivation and Context

This PR is intended to address the following cases of zulip

If it was set to false before the first await and then true after it, the first redraw should be skipped, but the second one shouldn't.

In #3020 , I didn't consider the case where event.redraw would later be true.

How Has This Been Tested?

npm run test with additional test

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.

dead-claudia

I missed this during the first review, but while we're at it, can you make sure to skip redraws after the component is removed, so removals don't trigger further redraws? (This bug also exists with sync event handlers, being able to fire events post-removal, and we should fix both in a single step.)

You should be able to get away with just doing this:

Add a if (self._ != null) check to the condition in the resolver handler where self is the outer event listener object.
Add an if (vnode.events != null) vnode.events._ = null right before this line.

That'd fix both.

kfule · 2025-05-09T10:28:49Z

@dead-claudia Thanks for your review.
As you suggested, I have changed the code so that it does not redraw after removing a dom or component (with the tests.)

With this commit, I removed the eventRedraw variable that holds this._, because self holds this. (i.e., changed the code to look like the old (0, this._) ())

kfule · 2025-05-09T14:53:42Z

@dead-claudia
This is not directly related to this PR, but it appears that the number of assertions in Test and maybe release / run-tests / test-node is the number of main branch (16789 assertions) and not the number of branches in this PR (just 16800 assertions).
The appropriate branch may not have been checked out in the test Actions.

I was curious about the difference in the number of assertions, so I rebased the commits just to be sure, but the result was not changed.

Edit: The number of assertions may have been different for #3020.

dead-claudia · 2025-05-09T19:34:04Z

@dead-claudia

This is not directly related to this PR, but it appears that the number of assertions in Test and maybe release / run-tests / test-node is the number of main branch (16789 assertions) and not the number of branches in this PR (just 16800 assertions).

The appropriate branch may not have been checked out in the test Actions.

That's a good catch. Filed MithrilJS/infra#69 to track it down. (This is in a common workflow script.)

dead-claudia

See comments.

Should hopefully be the last request for changes.

dead-claudia · 2025-05-09T19:42:16Z

render/tests/test-event.js

@@ -884,4 +916,48 @@ o.spec("event", function() {
 			})
 		})
 	})
+	o("avoid sync redraw after removal", function() {


Can you extend this test to also verify the event listener itself wasn't called?

That way, we know it couldn't possibly attempt an async redraw later.

I added assertions about whether the event listener itself was called or not.

The event listener itself is not removed and will be called, but it should be clearer that redraw is not being called.

dead-claudia · 2025-05-09T19:49:36Z

render/tests/test-event.js

+		o(redraw.callCount).equals(1)
+		o(redraw.this).equals(undefined)
+		o(redraw.args.length).equals(0)


Can you also assert thenCB is set? That being set also implies that the event listener was actually called.

OK, added the following assertions.

Whether the event listener itself has been called or not.

Whether thenCB was set or not.

dead-claudia · 2025-05-09T19:50:12Z

render/tests/test-event.js

+		render(root, div)
+		div.dom.dispatchEvent(e)
+
+		o(redraw.callCount).equals(0)


Can you also assert thenCB is set? That being set also implies that the event listener was actually called.

This makes it clear that the event listener is being called but redraw is not being called.

kfule · 2025-05-09T23:56:55Z

@dead-claudia Thanks for your kind review.
I added assertions about whether the event listeners themselves were called or not where redraw would not be called.

Of course I have confirmed all tests pass (with my local npm run test).

kfule requested a review from a team as a code owner May 7, 2025 12:56

dead-claudia requested changes May 8, 2025

View reviewed changes

kfule requested a review from dead-claudia May 9, 2025 10:28

kfule added 2 commits May 9, 2025 23:27

Allow additional async redraw even if the first redraw is skipped

8449657

Avoid redraw after dom removal

8c3734b

kfule force-pushed the true-after-await branch from 2c5a545 to 8c3734b Compare May 9, 2025 14:28

dead-claudia mentioned this pull request May 9, 2025

PR tests are, in some circumstances, checking out the main branch rather than the PR branch MithrilJS/infra#69

Open

dead-claudia requested changes May 9, 2025

View reviewed changes

Add assertion of whether an event listener has been called

9bb7648

This makes it clear that the event listener is being called but redraw is not being called.

kfule requested a review from dead-claudia May 9, 2025 23:52

dead-claudia approved these changes May 10, 2025

View reviewed changes

dead-claudia merged commit a239c77 into MithrilJS:main May 10, 2025
7 checks passed

JAForbes mentioned this pull request May 10, 2025

Release - v2.3.0 #3016

Merged

kfule deleted the true-after-await branch May 28, 2025 13:25

dead-claudia mentioned this pull request May 29, 2025

Missing docs for async event listeners MithrilJS/docs#34

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Allow additional async redraw even if the first redraw is skipped #3021

Allow additional async redraw even if the first redraw is skipped #3021

kfule commented May 7, 2025 •

edited

Loading

Uh oh!

dead-claudia left a comment

Uh oh!

kfule commented May 9, 2025 •

edited

Loading

Uh oh!

kfule commented May 9, 2025 •

edited

Loading

Uh oh!

dead-claudia commented May 9, 2025

Uh oh!

dead-claudia left a comment

Uh oh!

dead-claudia May 9, 2025

Uh oh!

kfule May 9, 2025

Uh oh!

dead-claudia May 9, 2025

Uh oh!

kfule May 9, 2025

Uh oh!

dead-claudia May 9, 2025

Uh oh!

kfule May 10, 2025

Uh oh!

kfule commented May 9, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Allow additional async redraw even if the first redraw is skipped #3021

Allow additional async redraw even if the first redraw is skipped #3021

Conversation

kfule commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist

Uh oh!

dead-claudia left a comment

Choose a reason for hiding this comment

Uh oh!

kfule commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfule commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dead-claudia commented May 9, 2025

Uh oh!

dead-claudia left a comment

Choose a reason for hiding this comment

Uh oh!

dead-claudia May 9, 2025

Choose a reason for hiding this comment

Uh oh!

kfule May 9, 2025

Choose a reason for hiding this comment

Uh oh!

dead-claudia May 9, 2025

Choose a reason for hiding this comment

Uh oh!

kfule May 9, 2025

Choose a reason for hiding this comment

Uh oh!

dead-claudia May 9, 2025

Choose a reason for hiding this comment

Uh oh!

kfule May 10, 2025

Choose a reason for hiding this comment

Uh oh!

kfule commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kfule commented May 7, 2025 •

edited

Loading

kfule commented May 9, 2025 •

edited

Loading

kfule commented May 9, 2025 •

edited

Loading

kfule commented May 9, 2025 •

edited

Loading