feat: Redraw once promise returned from event handler is settled #2862

EtiamNullam · 2023-08-18T22:17:26Z

Description

In this PR I'm proposing a solution to handle async event handlers.

I've decided to duck-type for finally with fallback to then. I assume we still want to redraw on rejected promise.

NOTE: seems like onbeforeremove lifecycle 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

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)
Documentation change

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I have updated docs/changelog.md

EDIT: 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.

Bumps [minimatch](https://github.com/isaacs/minimatch) from 3.0.4 to 3.1.2. - [Release notes](https://github.com/isaacs/minimatch/releases) - [Changelog](https://github.com/isaacs/minimatch/blob/main/changelog.md) - [Commits](isaacs/minimatch@v3.0.4...v3.1.2) --- updated-dependencies: - dependency-name: minimatch dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [async](https://github.com/caolan/async) from 2.6.3 to 2.6.4. - [Release notes](https://github.com/caolan/async/releases) - [Changelog](https://github.com/caolan/async/blob/v2.6.4/CHANGELOG.md) - [Commits](caolan/async@v2.6.3...v2.6.4) --- updated-dependencies: - dependency-name: async dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: Claudia Meadows <[email protected]>

…/npm)

Not sure how I forgot about this when I added the method.

Bumps [yaml](https://github.com/eemeli/yaml) to 2.2.2 and updates ancestor dependency [lint-staged](https://github.com/okonet/lint-staged). These dependencies need to be updated together. Updates `yaml` from 1.10.2 to 2.2.2 - [Release notes](https://github.com/eemeli/yaml/releases) - [Commits](eemeli/yaml@v1.10.2...v2.2.2) Updates `lint-staged` from 12.3.4 to 13.2.1 - [Release notes](https://github.com/okonet/lint-staged/releases) - [Commits](lint-staged/lint-staged@v12.3.4...v13.2.1) --- updated-dependencies: - dependency-name: yaml dependency-type: indirect - dependency-name: lint-staged dependency-type: direct:development ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [word-wrap](https://github.com/jonschlinkert/word-wrap) from 1.2.3 to 1.2.4. - [Release notes](https://github.com/jonschlinkert/word-wrap/releases) - [Commits](jonschlinkert/word-wrap@1.2.3...1.2.4) --- updated-dependencies: - dependency-name: word-wrap dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>

dead-claudia · 2023-09-01T19:46:28Z

render/render.js

+ if (this._ && ev.redraw !== false) {
+ (0, this._)()
+ if (result != null) {
+ if (typeof result.finally === "function") result.finally((0, this._))


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.

dead-claudia · 2023-09-01T19:49:58Z

render/render.js

+ (0, this._)()
+ if (result != null) {
+ if (typeof result.finally === "function") result.finally((0, this._))
+ else if (typeof result.then === "function") result.then((0, this._))


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 finally to ensure errors get reported correctly. (Obviously, you can't use async/await, but it's fairly straightforward.)

I was not sure about the use of this (0, expr) so I've tried to keep it consistent. I'm gonna call this._ directly.

Not sure what you mean by "my above finally" but I guess I can just pass this._ to both resolve and reject callbacks of then, 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!

EtiamNullam · 2023-10-08T09:04:58Z

Let me know if there's anything more I have to do. I've applied the requested changes.

JAForbes

I'm in favour of this change. I think it would be great to have in 2.x. If we support this I think it makes sense to have the same behaviour for oncreate / onupdate etc too. It would be surprising to only redraw on dom methods but not lifecycle methods.

JAForbes · 2024-09-23T15:15:41Z

docs/autoredraw.md

@@ -45,6 +45,61 @@ function doSomething(e) {
 m.mount(document.body, MyComponent)
 ```

+Mithril.js also redraws once promise returned from event handler resolves. It makes it easy to work with async code.


I think this might be clearer:

If you return a promise from an event handler, Mithril.js will redraw when it settles.

JAForbes · 2024-09-23T15:19:51Z

docs/autoredraw.md

+ }
+}
+
+m.mount(document.body, ComplexDelayedCounterComponent)


It would be great if the example was a bit more practical ie making a network request and changing state on success/fail in the event handler.

Also I think it would be great to inline the async handler ie

m('button', { async onclick(){ await m.request(...) // etc } })

It makes it clear that mithril is natively supporting async methods in the hyperscript.

JAForbes · 2024-09-25T05:28:50Z

@dead-claudia was this intentionally closed, or just a side effect of the base branch being deleted?

dead-claudia · 2024-09-25T05:31:45Z

@JAForbes It's the latter. They'll all need re-opened. Thanks for the catch! (I'm going to take a short break, then clean all that mess up by re-targeting them all.)

dead-claudia · 2024-09-25T08:58:49Z

@JAForbes Turns out you can't change the base of a closed branch, even if you try to use the REST API to atomically reopen it.

$ gh api --method PATCH -H 'Accept: application/vnd.github+json' -H 'X-GitHub-Api-Version: 2022-11-28' /repos/MithrilJS/mithril.js/pulls/2824 -f state=open -f base=main
{
  "message": "Validation Failed",
  "errors": [
    {
      "message": "Cannot change the base branch of a closed pull request.",
      "resource": "PullRequest",
      "field": "base",
      "code": "invalid"
    }
  ],
  "documentation_url": "https://docs.github.com/rest/pulls/pulls#update-a-pull-request",
  "status": "422"
}
gh: Validation Failed (HTTP 422)

So this (and all the others) would have to be recreated. 😕

I logged a feature request for them to at least allow that from the API: https://github.com/orgs/community/discussions/139697

dependabot bot and others added 14 commits February 7, 2023 04:59

hyperscript: handles shared empty attrs, fixes MithrilJS#2821

88da9e5

Fix typos in stream() docs (MithrilJS#2825)

ad9aa97

Co-authored-by: Claudia Meadows <[email protected]>

docs: removed question mark and fixed not working anchor link (github…

8cce14c

…/npm)

docs: fix regex for parsing page title

2b897c4

Add missing m.censor to API navigation

3c77beb

Not sure how I forgot about this when I added the method.

fix strange behavior, bump marked.js version up

1a12c67

docs: make url absolute in version selector

ee7dcdf

feat: Redraw once promise returned from event handler is settled

e2964f9

docs: Document async event handlers

1638059

test: Test async event handlers

7995ae9

EtiamNullam marked this pull request as ready for review August 21, 2023 02:56

EtiamNullam requested review from dead-claudia and StephanHoyer as code owners August 21, 2023 02:56

dead-claudia requested changes Sep 1, 2023

View reviewed changes

EtiamNullam added 3 commits September 3, 2023 20:49

fix: Don't attach to finally in async event handlers

1efe6c7

fix: Don't use redundant (0, this._), redraw on rejection

9ea34be

fix: Propagate unhandled errors thrown in event handlers

5cb2b4e

EtiamNullam requested a review from dead-claudia September 17, 2023 20:55

dead-claudia mentioned this pull request Sep 21, 2023

Move from return false to m.capture(event) for event handlers #2681

Closed

dead-claudia added Type: Enhancement For any feature request or suggestion that isn't a bug fix Area: Core For anything dealing with Mithril core itself labels Sep 2, 2024

dead-claudia force-pushed the next branch from 1e05bd3 to a4bf63d Compare September 23, 2024 11:58

dead-claudia requested a review from a team as a code owner September 23, 2024 11:58

JAForbes reviewed Sep 23, 2024

View reviewed changes

dead-claudia deleted the branch MithrilJS:next September 25, 2024 05:23

dead-claudia closed this Sep 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Redraw once promise returned from event handler is settled #2862

feat: Redraw once promise returned from event handler is settled #2862

EtiamNullam commented Aug 18, 2023 •

edited

Loading

dead-claudia Sep 1, 2023

dead-claudia Sep 1, 2023

EtiamNullam Sep 3, 2023

EtiamNullam commented Oct 8, 2023

JAForbes left a comment

JAForbes Sep 23, 2024

JAForbes Sep 23, 2024

JAForbes commented Sep 25, 2024

dead-claudia commented Sep 25, 2024

dead-claudia commented Sep 25, 2024

feat: Redraw once promise returned from event handler is settled #2862

feat: Redraw once promise returned from event handler is settled #2862

Conversation

EtiamNullam commented Aug 18, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

dead-claudia Sep 1, 2023

Choose a reason for hiding this comment

dead-claudia Sep 1, 2023

Choose a reason for hiding this comment

EtiamNullam Sep 3, 2023

Choose a reason for hiding this comment

EtiamNullam commented Oct 8, 2023

JAForbes left a comment

Choose a reason for hiding this comment

JAForbes Sep 23, 2024

Choose a reason for hiding this comment

JAForbes Sep 23, 2024

Choose a reason for hiding this comment

JAForbes commented Sep 25, 2024

dead-claudia commented Sep 25, 2024

dead-claudia commented Sep 25, 2024

EtiamNullam commented Aug 18, 2023 •

edited

Loading