-
-
Notifications
You must be signed in to change notification settings - Fork 926
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
feat: Redraw once promise returned from event handler is settled #2862
feat: Redraw once promise returned from event handler is settled #2862
Conversation
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] <support@github.com>
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] <support@github.com>
Co-authored-by: Claudia Meadows <contact@claudiameadows.dev>
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] <support@github.com>
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] <support@github.com>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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 useasync
/await
, but it's fairly straightforward.)
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 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 bothresolve
andreject
callbacks 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. |
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'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.
@@ -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. |
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 think this might be clearer:
If you return a promise from an event handler, Mithril.js will redraw when it settles.
} | ||
} | ||
|
||
m.mount(document.body, ComplexDelayedCounterComponent) |
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.
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.
@dead-claudia was this intentionally closed, or just a side effect of the base branch being deleted? |
@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.) |
@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.
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 |
Description
In this PR I'm proposing a solution to handle async event handlers.
I've decided to duck-type for
finally
with fallback tothen
. 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
Checklist:
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.