From: Dominik Csapak <d.csapak@proxmox.com>
To: Gabriel Goller <g.goller@proxmox.com>,
Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox-backup 1/2] gui: remove document.execCommand calls
Date: Thu, 14 Mar 2024 10:13:31 +0100 [thread overview]
Message-ID: <ca991366-70fa-4715-a52d-3fde48b4ad35@proxmox.com> (raw)
In-Reply-To: <CZTCS4F7CR0R.VTGJIS76FIKQ@proxmox.com>
On 3/14/24 10:08, Gabriel Goller wrote:
> On Thu Mar 14, 2024 at 8:59 AM CET, Dominik Csapak wrote:
>> hi,
>>
>> just to note: changing to an async function can be rather dangerous,
>> sometimes extjs not only calls the handlers/events/etc. but does things
>> after them (expecting the function to be finished) or waits for the return value.
>
> Hmm, but does it actually **await** the return value? I'm not a ext.js
> expert but I don't think it is. Anyways it's fine because js promises
> are executed eagerly + promise returned directly.
no it does not awaits anything and thats exactly the problem,
for example (not a real one)
extjs expects a callback foo
after it calls it, it expects foo to be finished to do further work
(e.g. a cleanup after an event handler)
we give an async function as foo, with multiple await calls in there
now it's not guaranteed the callback finished when extjs expects it to
>
>> (most of the extjs code was from before async/await was a thing in js)
>>
>> so here it seems to work out fine, but we have to be careful with
>> sprinkling async function in the code, otherwise we'll get
>> very unexpected results
>>
>> in general, i'd like to see that mentioned in the commit message
>> why it's ok to do that (no hard feelings though)
>>
>> otherwise LGTM
>
> What do you think about adding this to the commit message:
>
> Making the handler functions async is not a problem, because
> promises in js are executed eagerly (not lazily) and nothing
> depends/waits on the result of this handler.
as i mentioned above i don't think that's the reason why it's okay.
it should be okay because extjs executes the handler and does not expect
any result from it, nor does it need to do some work afterwards
next prev parent reply other threads:[~2024-03-14 9:14 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-13 16:17 Gabriel Goller
2024-03-13 16:17 ` [pbs-devel] [PATCH proxmox-backup 2/2] fix #5188: gui: add copy to clipboard on snapshots Gabriel Goller
2024-03-14 8:01 ` Dominik Csapak
2024-03-14 9:10 ` Gabriel Goller
2024-03-14 7:59 ` [pbs-devel] [PATCH proxmox-backup 1/2] gui: remove document.execCommand calls Dominik Csapak
2024-03-14 9:08 ` Gabriel Goller
2024-03-14 9:13 ` Dominik Csapak [this message]
2024-03-14 9:21 ` Gabriel Goller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ca991366-70fa-4715-a52d-3fde48b4ad35@proxmox.com \
--to=d.csapak@proxmox.com \
--cc=g.goller@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox