From 7f802631c54d2e91301158380b273b872d62bd80 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 12 Nov 2021 20:37:45 +0800 Subject: [PATCH] Fix some incorrect async functions, improve frontend document. (#17597) --- .../doc/developers/guidelines-frontend.md | 59 +++++++++++++++++++ web_src/js/features/clipboard.js | 22 ++++--- web_src/js/features/notification.js | 33 ++++++----- web_src/js/features/repo-graph.js | 19 +++--- web_src/js/features/repo-legacy.js | 2 +- web_src/js/features/repo-projects.js | 4 +- web_src/js/features/stopwatch.js | 10 ++-- 7 files changed, 106 insertions(+), 43 deletions(-) diff --git a/docs/content/doc/developers/guidelines-frontend.md b/docs/content/doc/developers/guidelines-frontend.md index f30b0d1cb..c937cfb7b 100644 --- a/docs/content/doc/developers/guidelines-frontend.md +++ b/docs/content/doc/developers/guidelines-frontend.md @@ -39,6 +39,65 @@ We recommend [Google HTML/CSS Style Guide](https://google.github.io/styleguide/h 6. The backend can pass complex data to the frontend by using `ctx.PageData["myModuleData"] = map[]{}` 7. Simple pages and SEO-related pages use Go HTML Template render to generate static Fomantic-UI HTML output. Complex pages can use Vue2 (or Vue3 in future). + +### `async` Functions + +Only mark a function as `async` if and only if there are `await` calls +or `Promise` returns inside the function. + +It's not recommended to use `async` event listeners, which may lead to problems. +The reason is that the code after await is executed outside the event dispatch. +Reference: https://github.com/github/eslint-plugin-github/blob/main/docs/rules/async-preventdefault.md + +If we want to call an `async` function in a non-async context, +it's recommended to use `const _promise = asyncFoo()` to tell readers +that this is done by purpose, we want to call the async function and ignore the Promise. +Some lint rules and IDEs also have warnings if the returned Promise is not handled. + +#### DOM Event Listener + +```js +el.addEventListener('click', (e) => { + (async () => { + await asyncFoo(); // recommended + // then we shound't do e.preventDefault() after await, no effect + })(); + + const _promise = asyncFoo(); // recommended + + e.preventDefault(); // correct +}); + +el.addEventListener('async', async (e) => { // not recommended but acceptable + e.preventDefault(); // acceptable + await asyncFoo(); // skip out event dispath + e.preventDefault(); // WRONG +}); +``` + +#### jQuery Event Listener + +```js +$('#el').on('click', (e) => { + (async () => { + await asyncFoo(); // recommended + // then we shound't do e.preventDefault() after await, no effect + })(); + + const _promise = asyncFoo(); // recommended + + e.preventDefault(); // correct + return false; // correct +}); + +$('#el').on('click', async (e) => { // not recommended but acceptable + e.preventDefault(); // acceptable + return false; // WRONG, jQuery expects the returned value is a boolean, not a Promise + await asyncFoo(); // skip out event dispath + return false; // WRONG +}); +``` + ### Vue2/Vue3 and JSX Gitea is using Vue2 now, we plan to upgrade to Vue3. We decided not to introduce JSX to keep the HTML and the JavaScript code separated. diff --git a/web_src/js/features/clipboard.js b/web_src/js/features/clipboard.js index 8d28b4e28..89aface93 100644 --- a/web_src/js/features/clipboard.js +++ b/web_src/js/features/clipboard.js @@ -46,7 +46,7 @@ function fallbackCopyToClipboard(text) { } export default function initGlobalCopyToClipboardListener() { - document.addEventListener('click', async (e) => { + document.addEventListener('click', (e) => { let target = e.target; // in case , so we just search up to 3 levels for performance. for (let i = 0; i < 3 && target; i++) { @@ -58,16 +58,20 @@ export default function initGlobalCopyToClipboardListener() { } if (text) { e.preventDefault(); - try { - await navigator.clipboard.writeText(text); - onSuccess(target); - } catch { - if (fallbackCopyToClipboard(text)) { + + (async() => { + try { + await navigator.clipboard.writeText(text); onSuccess(target); - } else { - onError(target); + } catch { + if (fallbackCopyToClipboard(text)) { + onSuccess(target); + } else { + onError(target); + } } - } + })(); + break; } target = target.parentElement; diff --git a/web_src/js/features/notification.js b/web_src/js/features/notification.js index f4c31c5ed..1e483b16c 100644 --- a/web_src/js/features/notification.js +++ b/web_src/js/features/notification.js @@ -3,21 +3,22 @@ const {appSubUrl, csrfToken, notificationSettings} = window.config; let notificationSequenceNumber = 0; export function initNotificationsTable() { - $('#notification_table .button').on('click', async function () { - const data = await updateNotification( - $(this).data('url'), - $(this).data('status'), - $(this).data('page'), - $(this).data('q'), - $(this).data('notification-id'), - ); - - if ($(data).data('sequence-number') === notificationSequenceNumber) { - $('#notification_div').replaceWith(data); - initNotificationsTable(); - } - await updateNotificationCount(); + $('#notification_table .button').on('click', function () { + (async () => { + const data = await updateNotification( + $(this).data('url'), + $(this).data('status'), + $(this).data('page'), + $(this).data('q'), + $(this).data('notification-id'), + ); + if ($(data).data('sequence-number') === notificationSequenceNumber) { + $('#notification_div').replaceWith(data); + initNotificationsTable(); + } + await updateNotificationCount(); + })(); return false; }); } @@ -104,8 +105,8 @@ export function initNotificationCount() { } const fn = (timeout, lastCount) => { - setTimeout(async () => { - await updateNotificationCountWithCallback(fn, timeout, lastCount); + setTimeout(() => { + const _promise = updateNotificationCountWithCallback(fn, timeout, lastCount); }, timeout); }; diff --git a/web_src/js/features/repo-graph.js b/web_src/js/features/repo-graph.js index 007cf9b38..73bde5fac 100644 --- a/web_src/js/features/repo-graph.js +++ b/web_src/js/features/repo-graph.js @@ -48,7 +48,7 @@ export default function initRepoGraphGit() { }); const url = new URL(window.location); const params = url.searchParams; - const updateGraph = async () => { + const updateGraph = () => { const queryString = params.toString(); const ajaxUrl = new URL(url); ajaxUrl.searchParams.set('div-only', 'true'); @@ -57,14 +57,15 @@ export default function initRepoGraphGit() { $('#rel-container').addClass('hide'); $('#rev-container').addClass('hide'); $('#loading-indicator').removeClass('hide'); - - const div = $(await $.ajax(String(ajaxUrl))); - $('#pagination').html(div.find('#pagination').html()); - $('#rel-container').html(div.find('#rel-container').html()); - $('#rev-container').html(div.find('#rev-container').html()); - $('#loading-indicator').addClass('hide'); - $('#rel-container').removeClass('hide'); - $('#rev-container').removeClass('hide'); + (async () => { + const div = $(await $.ajax(String(ajaxUrl))); + $('#pagination').html(div.find('#pagination').html()); + $('#rel-container').html(div.find('#rel-container').html()); + $('#rev-container').html(div.find('#rev-container').html()); + $('#loading-indicator').addClass('hide'); + $('#rel-container').removeClass('hide'); + $('#rev-container').removeClass('hide'); + })(); }; const dropdownSelected = params.getAll('branch'); if (params.has('hide-pr-refs') && params.get('hide-pr-refs') === 'true') { diff --git a/web_src/js/features/repo-legacy.js b/web_src/js/features/repo-legacy.js index f4a8c0cf3..8945360cd 100644 --- a/web_src/js/features/repo-legacy.js +++ b/web_src/js/features/repo-legacy.js @@ -351,6 +351,7 @@ export function initRepository() { // Edit issue or comment content $(document).on('click', '.edit-content', async function (event) { + event.preventDefault(); $(this).closest('.dropdown').find('.menu').toggle('visible'); const $segment = $(this).closest('.header').next(); const $editContentZone = $segment.find('.edit-content-zone'); @@ -511,7 +512,6 @@ export function initRepository() { $textarea.focus(); $simplemde.codemirror.focus(); }); - event.preventDefault(); }); initRepoIssueCommentDelete(); diff --git a/web_src/js/features/repo-projects.js b/web_src/js/features/repo-projects.js index 995b971be..270d54671 100644 --- a/web_src/js/features/repo-projects.js +++ b/web_src/js/features/repo-projects.js @@ -63,9 +63,7 @@ export default function initRepoProject() { return; } - (async () => { - await initRepoProjectSortable(); - })(); + const _promise = initRepoProjectSortable(); $('.edit-project-board').each(function () { const projectHeader = $(this).closest('.board-column-header'); diff --git a/web_src/js/features/stopwatch.js b/web_src/js/features/stopwatch.js index ff3edaf8c..f6185f7ff 100644 --- a/web_src/js/features/stopwatch.js +++ b/web_src/js/features/stopwatch.js @@ -82,8 +82,8 @@ export function initStopwatch() { } const fn = (timeout) => { - setTimeout(async () => { - await updateStopwatchWithCallback(fn, timeout); + setTimeout(() => { + const _promise = updateStopwatchWithCallback(fn, timeout); }, timeout); }; @@ -122,7 +122,7 @@ async function updateStopwatch() { return updateStopwatchData(data); } -async function updateStopwatchData(data) { +function updateStopwatchData(data) { const watch = data[0]; const btnEl = $('.active-stopwatch-trigger'); if (!watch) { @@ -135,14 +135,14 @@ async function updateStopwatchData(data) { $('.stopwatch-cancel').attr('action', `${issueUrl}/times/stopwatch/cancel`); $('.stopwatch-issue').text(`${repo_owner_name}/${repo_name}#${issue_index}`); $('.stopwatch-time').text(prettyMilliseconds(seconds * 1000)); - await updateStopwatchTime(seconds); + updateStopwatchTime(seconds); btnEl.removeClass('hidden'); } return !!data.length; } -async function updateStopwatchTime(seconds) { +function updateStopwatchTime(seconds) { const secs = parseInt(seconds); if (!Number.isFinite(secs)) return;