Skip to content
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

Wrong async/await/preventDefault usage #17448

Closed
wxiaoguang opened this issue Oct 27, 2021 · 5 comments · Fixed by #17386 or #17597
Closed

Wrong async/await/preventDefault usage #17448

wxiaoguang opened this issue Oct 27, 2021 · 5 comments · Fixed by #17386 or #17597
Labels
topic/ui Change the appearance of the Gitea UI type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 27, 2021

This is an issue for discussion of frontend refactoring. @lafriks @silverwind

The code https://github.com/go-gitea/gitea/blob/main/web_src/js/features/repo-legacy.js#L350 works like this:

$(document).on('click', '.edit-content', async function (event) {
    await createDropzone();
    event.preventDefault();
});

// equal to 
$(document).on('click', '.edit-content', function (event) {
    return Promise.resolve(createDropzone).then(() => {
        event.preventDefault();
    });
});

The usage of async/await is incorrect. The preventDefault won't have effect because the event has been processed synchronously before (when the listener returned).

So, that's why I insist to clean up most inconsistent await/async in the code base, only use them when it is necessary.

We should do our best to make everything work as expected.

image

@lunny lunny added type/proposal The new feature has not been accepted yet but needs to be discussed first. topic/ui Change the appearance of the Gitea UI labels Oct 27, 2021
@silverwind
Copy link
Member

Yes, this preventDefault won't work, that is certainly an oversight and there even seems to be an eslint plugin which we could potentially use to detect such issues.

My point thought is that anything that involved loading of additional JS chunks (await import()) should actually be awaited on because subsequent code might depend on the import actually being done loading, just firing off a Promise without awaiting it should only be done sparingly, if at all.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Oct 27, 2021

await import() is the correct usage and I keep them during refactoring, and we encourage to use them

Including await $.ajax()..., they are all good usages.

However the loading should only affect the code which uses the modules. We do not need to do await in global init stage, just leave these async/await calls to separate modules.

Generally speaking I think the code I refactored is on the right way.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Oct 27, 2021

ps: the methods mentioned in "Examples of correct code for this rule" of https://github.com/github/eslint-plugin-github/blob/main/docs/rules/async-preventdefault.md , are the methods I used during refactoring.

For exmple, this one: https://github.com/go-gitea/gitea/pull/17386/files#diff-1b029e915c1ca8d9b0d09ef0ce86797a36fbdca25ab233f179c14987a3bb0a05R137

(async () => {
...
})();

@silverwind
Copy link
Member

Yeah, async IIFE seems to be the most elegant way to split a function into sync/async parts.

@wxiaoguang
Copy link
Contributor Author

Can you help to review the PR #17386 again? I hope it can be approved soon to continue other works.

At least, it doesn't make thing worse than before.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
topic/ui Change the appearance of the Gitea UI type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
3 participants