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

Fix some incorrect async functions, improve frontend document. #17597

Merged
merged 7 commits into from
Nov 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions docs/content/doc/developers/guidelines-frontend.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
22 changes: 13 additions & 9 deletions web_src/js/features/clipboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 <button data-clipboard-text><svg></button>, so we just search up to 3 levels for performance.
for (let i = 0; i < 3 && target; i++) {
Expand All @@ -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;
Expand Down
35 changes: 18 additions & 17 deletions web_src/js/features/notification.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
}
Expand Down Expand Up @@ -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);
};

Expand Down
19 changes: 10 additions & 9 deletions web_src/js/features/repo-graph.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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') {
Expand Down
2 changes: 1 addition & 1 deletion web_src/js/features/repo-legacy.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -511,7 +512,6 @@ export function initRepository() {
$textarea.focus();
$simplemde.codemirror.focus();
});
event.preventDefault();
});

initRepoIssueCommentDelete();
Expand Down
4 changes: 1 addition & 3 deletions web_src/js/features/repo-projects.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
10 changes: 5 additions & 5 deletions web_src/js/features/stopwatch.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ export function initStopwatch() {
}

const fn = (timeout) => {
setTimeout(async () => {
await updateStopwatchWithCallback(fn, timeout);
setTimeout(() => {
const _promise = updateStopwatchWithCallback(fn, timeout);
}, timeout);
};

Expand Down Expand Up @@ -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) {
Expand All @@ -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;

Expand Down