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

Remove jQuery AJAX from the diff functions #29743

Merged
merged 4 commits into from
Mar 14, 2024
Merged
Changes from 2 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
68 changes: 41 additions & 27 deletions web_src/js/features/repo-diff.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import {initViewedCheckboxListenerFor, countAndUpdateViewedFiles, initExpandAndC
import {initImageDiff} from './imagediff.js';
import {showErrorToast} from '../modules/toast.js';
import {submitEventSubmitter} from '../utils/dom.js';
import {POST, GET} from '../modules/fetch.js';

const {csrfToken, pageData, i18n} = window.config;
const {pageData, i18n} = window.config;

function initRepoDiffReviewButton() {
const $reviewBox = $('#review-box');
Expand Down Expand Up @@ -63,8 +64,9 @@ function initRepoDiffConversationForm() {
if (isSubmittedByButton && submitter.name) {
formData.append(submitter.name, submitter.value);
}
const formDataString = String(new URLSearchParams(formData));
const $newConversationHolder = $(await $.post($form.attr('action'), formDataString));

const response = await POST($form.attr('action'), {data: formData});
const $newConversationHolder = $(await response.text());
const {path, side, idx} = $newConversationHolder.data();

$form.closest('.conversation-holder').replaceWith($newConversationHolder);
Expand All @@ -75,7 +77,8 @@ function initRepoDiffConversationForm() {
}
$newConversationHolder.find('.dropdown').dropdown();
initCompReactionSelector($newConversationHolder);
} catch { // here the caught error might be a jQuery AJAX error (thrown by await $.post), which is not good to use for error message handling
} catch (error) {
console.error('Error:', error);
showErrorToast(i18n.network_error);
silverwind marked this conversation as resolved.
Show resolved Hide resolved
} finally {
$form.removeClass('is-loading');
Expand All @@ -89,15 +92,20 @@ function initRepoDiffConversationForm() {
const action = $(this).data('action');
const url = $(this).data('update-url');

const data = await $.post(url, {_csrf: csrfToken, origin, action, comment_id});

if ($(this).closest('.conversation-holder').length) {
const conversation = $(data);
$(this).closest('.conversation-holder').replaceWith(conversation);
conversation.find('.dropdown').dropdown();
initCompReactionSelector(conversation);
} else {
window.location.reload();
try {
const response = await POST(url, {data: new URLSearchParams({origin, action, comment_id})});
const data = await response.text();

if ($(this).closest('.conversation-holder').length) {
const conversation = $(data);
$(this).closest('.conversation-holder').replaceWith(conversation);
conversation.find('.dropdown').dropdown();
initCompReactionSelector(conversation);
} else {
window.location.reload();
}
} catch (error) {
console.error('Error:', error);
}
});
}
Expand Down Expand Up @@ -132,18 +140,18 @@ function onShowMoreFiles() {
initImageDiff();
}

export function loadMoreFiles(url) {
export async function loadMoreFiles(url) {
const $target = $('a#diff-show-more-files');
if ($target.hasClass('disabled') || pageData.diffFileInfo.isLoadingNewData) {
return;
}

pageData.diffFileInfo.isLoadingNewData = true;
$target.addClass('disabled');
$.ajax({
type: 'GET',
url,
}).done((resp) => {

try {
const response = await GET(url);
const resp = await response.text();
const $resp = $(resp);
// the response is a full HTML page, we need to extract the relevant contents:
// 1. append the newly loaded file list items to the existing list
Expand All @@ -152,10 +160,13 @@ export function loadMoreFiles(url) {
$('body').append($resp.find('script#diff-data-script'));

onShowMoreFiles();
}).always(() => {
} catch (error) {
console.error('Error:', error);
showErrorToast('An error occurred while loading more files.');
} finally {
$target.removeClass('disabled');
pageData.diffFileInfo.isLoadingNewData = false;
});
}
}

function initRepoDiffShowMore() {
Expand All @@ -167,7 +178,7 @@ function initRepoDiffShowMore() {
loadMoreFiles(linkLoadMore);
});

$(document).on('click', 'a.diff-load-button', (e) => {
$(document).on('click', 'a.diff-load-button', async (e) => {
e.preventDefault();
const $target = $(e.target);

Expand All @@ -178,19 +189,22 @@ function initRepoDiffShowMore() {
$target.addClass('disabled');

const url = $target.data('href');
$.ajax({
type: 'GET',
url,
}).done((resp) => {

try {
const response = await GET(url);
const resp = await response.text();

if (!resp) {
$target.removeClass('disabled');
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!resp) {
$target.removeClass('disabled');
return;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because finally has done that.

Copy link
Member Author

@yardenshoham yardenshoham Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe remove only $target.removeClass('disabled');? To not continue if resp is falsy

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done f7691ae

$target.parent().replaceWith($(resp).find('#diff-file-boxes .diff-file-body .file-body').children());
onShowMoreFiles();
}).fail(() => {
} catch (error) {
console.error('Error:', error);
} finally {
$target.removeClass('disabled');
});
Comment on lines -191 to -193
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, fail?
Interesting, this code was even more wrong than expected…

}
});
}

Expand Down
Loading