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.

Sign up for GitHub

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

Remove jQuery AJAX from the diff functions #29743

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aff0c56
Remove jQuery AJAX from the diff functions
yardenshoham Mar 12, 2024
148ed28
Apply suggestions from code review
yardenshoham Mar 12, 2024
f7691ae
Remove redundant line
yardenshoham Mar 14, 2024
e9f936e
Merge branch 'main' into repo-diff-jquery-ajax
yardenshoham Mar 14, 2024
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
69 changes: 41 additions & 28 deletions web_src/js/features/repo-diff.js
View file
Open in desktop
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,21 @@ 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;
}
$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