Problem/Motivation
Reduce the weight of the contextual JS, since it's used on most pages when user has access to it.
Steps to reproduce
Proposed resolution
change the jquery ajax call with this, seems to be enough
// Perform an AJAX request to let the server render the contextual links
// for each of the placeholders.
if (uncachedIDs.length > 0) {
const data = new URLSearchParams();
uncachedIDs.forEach((id) => data.append('ids[]', id));
uncachedTokens.forEach((id) => data.append('tokens[]', id));
const req = new Request(Drupal.url('contextual/render'), {
method: 'POST',
body: data,
});
fetch(req)
.then((res) => res.json())
.then((json) =>
Object.entries(json).forEach(([contextualID, html]) => {
// Store the metadata.
storage.setItem(`Drupal.contextual.${contextualID}`, html);
// If the rendered contextual links are empty, then the current
// user does not have permission to access the associated links:
// don't render anything.
if (html.length > 0) {
// Update the placeholders to contain its rendered contextual
// links. Usually there will only be one placeholder, but it's
// possible for multiple identical placeholders exist on the
// page (probably because the same content appears more than
// once).
$placeholders = $context.find(
`[data-contextual-id="${contextualID}"]`,
);
// Initialize the contextual links.
for (let i = 0; i < $placeholders.length; i++) {
initContextual($placeholders.eq(i), html);
}
}
}),
)
.catch((error) => console.warn(error));
}
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Comments
Comment #4
godotislateOK, put up the MR based on the suggestion in the description. I actually tried tweaking the JS a little to remove more jQuery from the snippet, but tests were failing, so decided to leave that for later.
Some tests are failing because of AJAX wait asserts, and those assertions are based on events triggered by jQuery AJAX. I removed some of the lines to see if the tests will pass, but I think it's caused other issues, since I'm guessing there likely needs to be some sort of waiting going on for the requests for finish. Can look at that later.
Also, can't remove
core/drupal.ajaxas a dependency of contextual/drupal.contextual-links yet, because there's a call to Drupal.ajax.bindAjaxLinks at the bottom of contextual.js.Comment #6
mstrelan commentedSome of the tests were random fails, I fixed a couple more, and there are some remaining.
Comment #7
nod_Tests are green, since we never used fetch directly in core before the test framework didn't know how to wait on it.
We should be able to find a way to make the ajax contextual link not a dependency. Does anyone has a example of this being of use? was it for quickedit back in the day maybe?
Comment #8
godotislateIn non-test modules, layout_builder.links.contextual.yml and settings_tray.links.contextual.yml both have
use-ajaxlinks.Comment #9
godotislateHad an idea on how to remove the dependency, pushed a commit to see if tests pass.
Comment #10
godotislateOK tests seem to pass and the library dependency was removed. I changed it to attach the library to the links render array if any link had the
use-ajaxclass, then used attachBehaviors in contexual.js on the new DOM element instead of Drupal.ajax.bindAjaxLinks.Comment #11
nod_Oh nice, and the perf test looks great with 27k of JS removed :D
Comment #12
nod_Checked it out more closely, we kind of have nested attachBehaviors call, that isn't ideal.
Instead of that we can just try to call the function "safely". So doing
and keeping the backend code that conditionaly load
core/drupal.ajaxshould be enoughComment #13
godotislateMade changes per #12.
Comment #14
smustgrave commentedLeaning on nod_ review here but his feedback appears to be addressed.
Comment #16
alexpottCommitted 8b522ad and pushed to 11.x. Thanks!
Comment #19
nod_exciting :) thanks!