Problem/Motivation

It is impossible to load additional JS libraries in an Ajax response and then call that code through Ajax commands, because Drupal.ajax does not guarantee that the JS library will already have loaded.

The issue is more apparent when there is an \Drupal\Core\Ajax\InsertCommand "insert" AJAX command containing <script> tags which should be loaded in a specific manner (as it would on a normal DOM load), but aren't due to how jQuery currently works, and will result in a crash when the scripts are ran out of order.

Quick Javascript snippet to showcase the current behaviour:

var $el = jQuery('#element').length ? jQuery('#element') : jQuery('<div id="element"></div>').appendTo('body');
// https://javascript.info/script-async-defer
// long.js should ideally be loaded first, before small.js is. But currently doesn't.
$el.html(`
<script src="https://javascript.info/article/script-async-defer/long.js?speed=1"></script>
<script src="https://javascript.info/article/script-async-defer/small.js"></script>
`);

References:

See #23 and #31 for further details.

Proposed resolution

Create a new add_js ajax command that uses the loadjs library to ensure all external JS resources are loaded before continuing execution.

Remaining tasks

User interface changes

None.

API changes

Ajax commands can now return Promises to ensure command execution will wait for the promise to be resolved before continuing execution.
The ajaxing property of Drupal.Ajax objects is now set to false after all ajax command have been executed. Previously it was set to false as soon as the request came back from the network.

See the long list of related issues.

Release notes snippet

The processing of Ajax commands has changed. Ajax commands can now return promises when they need to ensure some code has been executed before executing the next Ajax command in the list. This is used for the new add_js command that ensures Additional JS files have been loaded before executing the next Ajax command.

When altering the success method of a Drupal.Ajax object please make sure a Promise is returned to ensure proper execution.

Conditional comments for IE will not be honored for JavaScript files with the new add_js command, all JS files will be added regardless of the value of #browsers. There are no changes for conditional comments on CSS files they will still work on Drupal 9.

Original report by [username]

Hi,

Thanks for your module, it works perfect for me.

But I found there one small bug there: if all my .js files are mapped to the external CDN, ajax redirects doesn't execute. After some tests I found that CDN rewrites all js urls, even for ajax. So I attached a small patch that solved this problem for me.

Best regards,
Spleshka.

CommentFileSizeAuthor
#349 1988968-349-9.3.14.patch26.86 KBrubens.arjr
#338 interdiff-335-338-10.x.txt586 bytesnod_
#338 core-ajaxload-1988968-338-10.x.patch34.07 KBnod_
#338 interdiff-335-338.txt586 bytesnod_
#338 core-ajaxload-1988968-338.patch35.62 KBnod_
#335 core-ajaxload-1988968-335-10.x.patch34.07 KBnod_
#335 interdiff-327-335-10.x.txt4.78 KBnod_
#335 core-ajaxload-1988968-335.patch35.62 KBnod_
#335 interdiff-327-335.txt4.78 KBnod_
#327 interdiff-318-327-10.x.txt7.67 KBnod_
#327 core-ajaxload-1988968-327-10.x.patch34.08 KBnod_
#327 interdiff-318-327.txt7.83 KBnod_
#327 core-ajaxload-1988968-327.patch35.63 KBnod_
#318 core-ajaxload-1988968-318-10.x.patch33.9 KBnod_
#318 interdiff-310-318-10.x.txt1.47 KBnod_
#318 core-ajaxload-1988968-318.patch35.44 KBnod_
#318 interdiff-309-318.txt1.59 KBnod_
#310 interdiff-305-310-10.x.txt5.79 KBnod_
#310 core-ajaxload-1988968-310-10.x.patch33.99 KBnod_
#309 interdiff-305-309.txt6.65 KBnod_
#309 core-ajaxload-1988968-309.patch35.95 KBnod_
#305 core-ajaxload-1988968-305-10.x.patch32.48 KBnod_
#305 core-ajaxload-1988968-305.patch33.81 KBnod_
#305 interdiff-302-305.txt450 bytesnod_
#302 core-ajaxload-1988968-302-10.x.patch32.55 KBnod_
#302 core-ajaxload-1988968-302.patch33.89 KBnod_
#302 interdiff-301-302.txt1.6 KBnod_
#301 core-ajaxload-1988968-301-10.x.patch31.95 KBnod_
#301 core-ajaxload-1988968-301.patch33.29 KBnod_
#301 interdiff-300-301.txt787 bytesnod_
#300 core-ajaxload-1988968-300-10.x.patch31.94 KBnod_
#300 core-ajaxload-1988968-300.patch33.28 KBnod_
#299 core-ajaxload-1988968-299-10.x.patch31.96 KBnod_
#299 core-ajaxload-1988968-299.patch33.29 KBnod_
#299 interdiff-293-299.txt10.87 KBnod_
#298 core-ajaxload-1988968-293.patch32.5 KBnod_
#296 core-ajaxload-1988968-295.patch32.15 KBnod_
#295 interdiff-293-295.txt950 bytesnod_
#293 interdiff-292-293.txt4.36 KBnod_
#293 core-ajaxload-1988968-293.patch32.5 KBnod_
#292 interdiff-289-292.txt718 bytesnod_
#292 core-ajaxload-1988968-292.patch28.14 KBnod_
#290 interdiff-287-289.txt2.14 KBnod_
#290 core-ajaxload-1988968-289.patch27.44 KBnod_
#288 core-ajaxload-1988968-287.patch27.16 KBnod_
#287 interdiff-284-287.txt8.45 KBnod_
#285 core-ajaxload-1988968-284.patch25.31 KBnod_
#283 1988968-282.patch22.05 KBnoorulshameera
#281 1988968-281.patch20.55 KBMegha_kundar
#280 1988968-280.patch22 KBdawid_nawrot
#270 reroll_diff_1988968-268_1988968-270.txt11.49 KByogeshmpawar
#270 1988968-270.patch22.75 KByogeshmpawar
#268 1988968-268.patch22.9 KBagata.guc
#266 Screen Shot 2022-01-24 at 9.06.11 AM.png346.4 KBdrupalninja99
#261 1988968-261.patch22.89 KBmichael.barlow@mirumagency.com
#259 1988968-259.patch22.88 KBmichael.barlow@mirumagency.com
#258 1988968-258.patch21.45 KBmichael.barlow@mirumagency.com
#244 interdiff_228-244.txt5.08 KBpmagunia
#244 1988968-244.patch22.45 KBpmagunia
#243 1988968-243.patch27.99 KBpmagunia
#242 1988968-242.patch28.02 KBpmagunia
#236 1988968-236.patch28 KBviappidu
#235 1988968-235.patch22.47 KBviappidu
#234 1988968-228--9.2.x.patch28.16 KBDuaelFr
#228 interdiff_227-228.txt1.34 KBzrpnr
#228 1988968-228.patch28.96 KBzrpnr
#227 1988968-reroll-9.3.x-227.patch28.62 KBzrpnr
#218 1988968-218-92X.patch27.25 KBbnjmnm
#216 1988968-216.patch27.24 KBviappidu
#213 1988968-213.patch21.81 KBviappidu
#211 interdiff-1988968_195_211.txt15.15 KBKapilV
#211 1988968-211.patch21.79 KBKapilV
#195 core-ajaxload-1988968-195.patch27.11 KBczigor
#193 core-ajaxload-1988968-193.patch27.26 KBnod_
#193 interdiff-191-193.txt2.77 KBnod_
#191 interdiff-185-191.txt12.28 KBnod_
#191 core-ajaxload-1988968-191.patch27.48 KBnod_
#185 interdiff.txt3.39 KBnod_
#185 core-ajaxload-1988968-185.patch28.15 KBnod_
#183 core-ajaxload-1988968-183.patch29.14 KBnod_
#183 interdiff-180-183.txt1.06 KBnod_
#180 core-ajaxload-1988968-180.patch33.72 KBnod_
#180 core-ajaxload-1988968-180-test-only.patch6.13 KBnod_
#178 interdiff-1988968-176-178.txt1.33 KBmartin107
#178 1988968-178.patch27.58 KBmartin107
#176 core-ajaxload-1988968-176.patch27.6 KBnod_
#176 interdiff-173-176.txt15.18 KBnod_
#173 1988968-173.patch28.75 KBextect
#172 1988968-172-2.patch28.41 KBextect
#171 1988968-171.patch6.86 KBextect
#170 1988968-172.patch19.86 KBextect
#162 interdiff.txt8.88 KBolli
#162 1988968-162.patch27.99 KBolli
#160 interdiff.txt2.82 KBolli
#160 1988968-160.patch23.59 KBolli
#158 1988968-145-2.patch23.61 KBolli
#158 interdiff_137-145.txt365 bytesolli
#156 1988968-137-3.patch23.94 KBandriic
#145 1988968-137-2.patch23.53 KBPhil Wolstenholme
#137 1988968-137.patch23.53 KBjansete
#133 1988968-133.patch24.76 KBs.messaris
#130 1988968-130.patch29.34 KBjansete
#127 1988968-127.patch29.35 KBjansete
#125 1988968-125.patch29.37 KBjansete
#119 image.png7.87 KBkeithdoyle9
#115 d7-ajax-hack-1988968.patch1.07 KBttkaminski
#113 1988968-113.patch29.49 KBmartin107
#105 1988968-105-ajax-loader.patch30.34 KBWim Leers
#105 interdiff.txt6.3 KBWim Leers
#103 interdiff-1988968-102-103.txt1.59 KBmartin107
#103 1988968-103-ajax-loader.patch28.85 KBmartin107
#102 1988968-102-ajax-loader.patch28.82 KBdroplet
#101 test-testbot.patch22.39 KBdroplet
#97 1988968-97-ajax.patch27.62 KBAda Hernandez
#97 interdiff-96-97.txt411 bytesAda Hernandez
#96 1988968-96-ajax.patch27.73 KBdroplet
#94 1988968-94-ajax.patch21.32 KBdroplet
#92 1988968-92-ajax.patch20.13 KBdroplet
#89 1988968-89.patch18.15 KBdroplet
#85 1988968-85.patch17.74 KBGrandmaGlassesRopeMan
#80 1988968-80-es6.patch17.3 KBdroplet
#78 1988968-78-es6.patch14.09 KBdroplet
#73 1988968-72.patch14.06 KBdroplet
#71 1988968-71.patch14.1 KBdroplet
#69 1988968-68.patch9.88 KBdroplet
#67 1988968-67.patch7.43 KBdroplet
#65 1988968-loadjs-65.patch7.95 KBdroplet
#63 1988968-loadjs-62.patch8.32 KBdroplet
#61 1988968-loadjs.patch6.03 KBdroplet
#48 droplet.html_.txt2.99 KBdroplet
#39 promisedAjaxLoader-1988968-39.patch5.82 KBdroplet
#37 promisedAjaxLoader.patch5.88 KBdroplet
#31 1988968-31.patch6.11 KBWim Leers
#31 Screen Shot 2016-09-26 at 15.38.31.png379.91 KBWim Leers
cdn-breaks-ajax-redirects.patch769 bytesSpleshka

Issue fork drupal-1988968

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Hrm… this seems odd.

So, what you're doing here, is essentially saying: "whenever file_create_url() executes on the system/ajax path, never let the CDN module alter any file URLs".

That doesn't make sense to me. Why wouldn't you e.g. image URLs that are served by system/ajax to be served from the CDN?

I greatly appreciate the patch, but in its current form it disables too much. I need to understand the problem better first :) Can you provide the specific problem, or set it up on a site that I can access so that I can see the problem myself?

Thanks! :)

Spleshka’s picture

Status: Needs review » Needs work

May be disable this on system/ajax path is too strict, but I have no idea how to fix it in the other way.

Example code that breaks:

module_load_include('inc', 'ctools', 'includes/ajax');
$commands = array();
$commands[] = ctools_ajax_command_redirect('some-url');
return array('#type' => 'ajax', '#commands' => $commands);

Basically, system/ajax processes only ajax requests so I believe that it will not break any content deliver through CDN. I'm not sure if problem only in redirects. We should dig deeper here. I'll try to do it as soon as possible.

Wim Leers’s picture

#2: and how does that example code break? Are you saying that some of your JS is somehow trying to access http://yourcdn.com/system/ajax?

Spleshka’s picture

I don't know what happens when ajax query runs. The only thing I know is that after ajax query is sent to system/ajax it gets lost somewhere and client has no response. So I want to look at this problem more carefully and provide a common solution.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Category: bug » support
Status: Needs work » Postponed (maintainer needs more info)

Well, in any case, your solution is too coarse. It disables the CDN module in unnecessarily many cases. Plus, if things were so terribly broken, then http://wimleers.com/demo/hierarchical-select/taxonomy could not work either (it also uses AJAX requests while the CDN module is enabled). I suspect the problem is more subtle than that.

Can you give me access to a site where this problem can be reproduced? Or can you provide me steps to reproduce this on a vanilla (fresh) Drupal installation?

Spleshka’s picture

I've made a vanilla sandbox for you. See http://sandbox.u2974.hank.vps-private.net/
Login / password: Wim Leers / sandbox.
If you need ssh/ftp access I can mail it to you.

To reproduce this issue I installed Ajax Register module. After you logged in the page should be refreshed. But if CDN is enabled the page is not refreshes.

Wim Leers’s picture

Sorry for the delay — DrupalCon Portland happened…

I'll try to look at this tomorrow.

Spleshka’s picture

I think I found the problem.

When using CDN all dynamic files are load after ajax commands is executed. I'm trying to say that sequence of actions for Drupal ajax when CDN is enabled or disabled is different. See this:

a. CDN IS DISABLED
1. ajax submit
2. get response
3. invoked first command Drupal.ajax.prototype.commands.settings() and merge with new Drupal.settings (that contains dynamic files)
4. new files are loaded
5. other ajax commands are invoked

b. CDN IS ENABLED
1. ajax submit
2. get response
3. invoked first command Drupal.ajax.prototype.commands.settings() and merge with new Drupal.settings (that contains dynamic files)
4. other ajax commands are invoked
5. new files are loaded

So when CDN is enabled and ajax response contains any command in a dynamically-loaded js file, it doesn't works because 5th step should be finished before 4th step is started.

Currently I don't know why is it occurs. I think that this may happen because of network latency. Or may be browser allows lazy load of scripts from external domains. I'm going to continue my research, but I'd appreciate any help.

Wim Leers’s picture

Title: CDN breaks AJAX redirects » Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS
Version: 7.x-2.x-dev » 8.x-dev
Component: Module » ajax system
Assigned: Spleshka » Unassigned
Category: support » bug
Priority: Normal » Major
Issue tags: +JavaScript

Thanks for the analysis!

I THINK your problem is this:

  1. The code previously only worked because files took a long time to load.
  2. The CDN ensures files load faster is apparently not a good one and makes those files take an even longer time to load.
  3. So now things break.

I've discussed this with nod_ (the JS maintainer) too. The thing is that Drupal.ajax has NO guarantees for this. Drupal.ajax not guaranteeing (nor being able to) that files are loaded before JS calls into them, because there is no way of knowing whether dependent libraries (additional JS that gets loaded) has actually been loaded? The CDN has made the loading of JS faster slower, and is hence breaking things…


Turning this into a Drupal core issue, which is what it really is. Affects all versions of Drupal. Should be fixed in 8 and if possible without changing APIs also in 7. Updated title, but it's a crappy one, so feel free to improve it.

nod_’s picture

We've been aware of core limitation around this. It's something I really really wished we wouldn't have to fix but alas.

Realistically looking at how D8 will be used, it's borderline critical.

It also mean we're digging up:
#1533366: Simplify and optimize Drupal.ajax() instantiation and implementation
#1033392: Script loader support in core (LABjs etc.)

And tangentially:
#1542344-63: Use AMD for JS architecture

Spleshka’s picture

Oh, it seems to me that I found something really big :) I think we all agree that this should be fixed before D8 release, because the solution this may lead to a big changes (hope not).

Please, just let me know if I can help you here. I'd love to spend some my weekends working on this.

Spleshka’s picture

Any progress in this issue? I remember that someone tries to figure out what to do first here.

jessebeach’s picture

If the dependency chain is just one file (or maybe even just two), we could use a Promise object, perhaps.

Adding a dependency management system to core at this point seems impossible, although if we want to do anything complicated with front end code in we'll need it.

Two modules exist to do this already:

drupal.org/project/labjs
drupal.org/project/requirejs

One of those will need an 8.x branch and some integration with Core. I see this as a dependency for building complex front end single page apps and the like.

I realize I'm not offering any solutions for this issue :)

Spleshka’s picture

I think that will not be the best solution. It is better to think about more elegant solution that we can implement into D7 core, and in the D8 too. Here is what I suggest:

Add to the array in ajax_command_*() functions a new key which will indicate js and css dependency. Example:

function ajax_command_invoke($selector, $method, array $arguments = array()) {
  return array(
    'command' => 'invoke',
    'selector' => $selector,
    'method' => $method,
    'arguments' => $arguments,
    'dependencies' => array(
      'js' => array('misc/ajax.js'),
      'css' => array(), // Just to show possibily.
    )
  );
}

Then before invoke each command we should check whether this files are loaded - Drupal.settings.ajaxPageState.js and Drupal.settings.ajaxPageState.css are contain this information. If some files are missed then just load them and add to the Drupal.settings.ajaxPageState.* variable. Or course, we should drop part with merging js and css into Drupal.settings on a backend side, because we will do it on the client side.

I think that this will not invoke major API changes and will be nicely applied to the 7x branch.

nod_’s picture

Then before invoke each command we should check whether this files are loaded - Drupal.settings.ajaxPageState.js and Drupal.settings.ajaxPageState.css are contain this information.

The key issue here is how?

Also what happens exactly when deps are not yet loaded?

Not trying to rain on your parade, just honest questions, I may have missed something.

Spleshka’s picture

Assigned: Unassigned » Spleshka

Okey, I think we need some code to appear here :) I'm going to provide a quick patch to show how I see this.

gilgabar’s picture

I'm running into this issue myself. My initial reaction was similar to what Wim stated in #15, but after digging into the issue a bit, it's clear that isn't correct.

If you configure your local dev environment so that your drupal site runs at a virtual host, something like 'test.dev' and then to simulate a cdn for testing purposes, you create a second virtual host that points to the exact same site, something like 'cdn.test.dev', you will see the exact same behavior as noted in #14. That rules out the performance of the cdn, or networking issues as a factor.

You can even eliminate drupal from the equation and still observe the same behavior. Drupal's ajax system adds the extra js assets via 'insert' commands which rely on jquery's .html() method to parse the inserted script tags and make the actual ajax requests for the assets. So if you create a simple jquery example that does something similar outside of drupal, you see the same thing.

// /jqery-test.html

<!doctype html>
<html lang="en">
<head>
  <meta charset="utf-8">
  <title>jQuery .html() ajax js loading demo</title>
  <script src="//code.jquery.com/jquery-1.4.4.js"></script>
</head>
<body>

<script>
(function ($) {
    console.log('Start.');

    // Change the url and refresh the page to switch between working/broken cases.
    var js_script = '\<script src=\"http://cdn.test.dev/jquery-test.js\"\>\</script\>';
    var new_content_wrapped = $('<div></div>').html(js_script);
    var new_content = new_content_wrapped.contents();
    $('head').prepend(new_content);

    // Run a function from the .html loaded js file.
    testfunction();

    console.log('Finish.');
})(jQuery);
</script>
 
</body>
</html>

And here is the simple js asset for it to load.

// /jquery-test.js

testfunction = function () {
  console.log('Ran the test function.');
}

Use the same virtual host config as noted above and observe the console. Change the domain of the js file and note that it runs the function from the js when the file shares the same domain with the page, but does not when they are different. You can rule out same origin issues as a factor if you wrap the testfunction() call in a setTimeout() as it is able to run after waiting a bit.

I haven't dug any deeper than that, so I don't have a complete explanation for what's happening. I suspect it is due to the way browsers load assets from different domains rather than something jquery is doing, but I could be wrong.

nod_’s picture

So basically here are the options on dynamic script loading: http://www.stevesouders.com/blog/2009/04/27/loading-scripts-without-bloc...

Spleshka’s picture

@nod_,

Do you have some time to digg into this issue? I'd love to help with solving this issue, but I'm not so js-addicted as you are :)

nod_’s picture

Oh :( I was counting on you, you assigned it to yourself back then. There are hundreds more issues I need to look at.

Even a quick & dirty patch would help.

cthshabel’s picture

I understand that the patch in this issue is not a viable solution; is there any progress expected for d7 (or potential workarounds)? I do see significant activity on d8 for this, but just curious if there are possible solutions for d7 now?

Wim Leers’s picture

Status: Active » Needs review
FileSize
379.91 KB
6.11 KB

This is still a problem in Drupal 8 too!

#23's analysis is spot-on. All that matters is indeed that it is a separate domain.

The question is: why do browsers behave this way? I was thinking it could be related to a connection still needing to be set up. But that's nonsense, because in the tests I'm doing, the page already has loaded lots of assets from the CDN domain. So, I still don't know why browsers are behaving this way, but there probably is a reason.

That being said, we should have firmer guarantees anyway. And it's definitely technically possible.

Here's an initial patch that demonstrates the problem. This is while using the CDN module to serve files from a separate domain (but just like in #23, this is just a different virtual host on the same machine — same performance, just a different domain). As soon as your JS is served from a a separate domain, you'll see that dialogs (for example the image or link dialog in CKEditor while creating a node at /node/add/page) stop working: the openDialog AJAX command is called before dialog.ajax.js has been loaded! Try to open a dialog again, and it'll work, because this time around, dialog.ajax.js has been loaded.


This patch takes a first stab at solving this. Instead of using the generic insert AJAX command, it introduces a add_js command (just like we already have a add_css command). It then loads JS in such a way that it can track which JS files have loaded. And so, "all" that we need to do, is to make the add_js AJAX command block on all JS files actually being loaded. That will then ensure that the openDialog AJAX command is called after dialog.ajax.js has been loaded.

However, making function calls block in JS is easier said than done. (I failed that part.) I think we'll need to change how Drupal.Ajax.prototype.success works, so that it doesn't simply iterate over all AJAX commands in an AJAX response and calls them, and assumes they all work. Instead, we need to make them run after each other (using promises/callbacks/…). The tricky part: this is technically an API break, which could break some JS out there that relies on Drupal.Ajax.prototype.success being a blocking call.

Wim Leers’s picture

This is a problem that's existed since the very beginning of the AJAX system. This is also why Drupal sites can sometimes have randomly broken JavaScript.

I'm tempted to mark this critical. This has not been receiving the attention it should have.

Wim Leers’s picture

#2714155: ajax-loading and attached libraries is what led me to think about this issue again. So the same issue has once again been reported.

nod_’s picture

The clean way to handle this would be to turn all ajax commands into promises and use that to ensure execution order (same thing with behaviors dependencies btw). But that means IE* is out and I don't know about polyfill quality on that topic.

We could try to change how commands are executed but it won't be rock solid.

Wim Leers’s picture

We can even get there with just callback chaining AFAICT. Or what am I missing?

nod_’s picture

Given the async nature of script loading, we'd be reimplementing promises essentially.

How to know a callback finished? it needs to explicitly say it finished so that other commands can run afterwards. You need a way to queue all those callbacks too. That's pretty much how promises work and what they were were made for. Unless we use a polyfill we're stuck.

droplet’s picture

How about this way? A Promise

droplet’s picture

Failed testing is expected while I added setTimeOut to simulate network delay. Let me upload one without it for testbots.

Wim Leers’s picture

Wim Leers’s picture

@droplet, @nod_: Should we implement this ourselves, or should we adopt LABjs? LABjs seems to do too much, we only want a subset of what it provides. Can we use a different JS library? Should we?

Wim Leers’s picture

I opened an issue for this in the CDN module issue queue: #2811615: [upstream] [Core bug, fixed since 9.5] AJAX commands that need additional JS to be loaded will fail when JS is loaded from CDN.

Note that you can also reproduce this without the CDN module: any site setting $settings['file_public_base_url'] will be affected by this. See https://www.drupal.org/node/2529168.

droplet’s picture

@Wim,

This is lightweight than LABjs.
https://github.com/ded/script.js (spfjs based on it)

Patch #39 is even more lightweight. (But yeah, it's cut off some features. and based on jQuery internal. And will not inject the script tag. )

In Patch #39, `it will not inject the script tag` - I don't know if there any side effects anyway. AFAIK, no :)

** Note that: failed tests in #39 are checking the script tags in HEAD.

Whatever we used, we can still expose an API `Drupal.drupalScriptLoader` to let other scripts using their own loader.

Wim Leers’s picture

Interesting. Sadly, it doesn't have a license yet. I left a comment at https://github.com/ded/script.js/issues/99.

droplet’s picture

FileSize
2.99 KB

haha. Just interesting the performance of my version above. Simplified a bit and use Script Tag.
https://jsfiddle.net/a2kj4j61/

Wim Leers’s picture

heddn’s picture

And also a while back (again with no CDN) in #2719083: Loading... ckeditor embed button

Wim Leers’s picture

Quarterly bump.

droplet’s picture

new rockstar:
https://github.com/muicss/loadjs

It's based on the $script.js and lightweight and MIT license.

Wim Leers’s picture

Oohh!

Wim Leers’s picture

LoadJS is a tiny async loader for modern browsers (710 bytes).

Wow, 710 bytes! :O

droplet’s picture

Can the PHP API changes in #39 commit to D8.4? Break BC?

If not, maybe there's some more modern robust workaround for D9. (don't waste time, skip 8.4 :P)

Wim Leers’s picture

bojanz’s picture

We had to work around this in Commerce as well (by having payment gateways declare their external libraries in their annotation, so that they're loaded by the parent form before #ajax)

Wim Leers’s picture

Glad to hear that in your case a work-around was possible. But it's not always an option…

Thanks for sharing/confirming!

Wim Leers’s picture

droplet’s picture

Status: Needs work » Needs review
FileSize
6.03 KB

testbots?

droplet’s picture

Status: Needs work » Needs review
FileSize
8.32 KB

Interesting, assets/vendor is ignored in my local...

droplet’s picture

remove console.log

droplet’s picture

Status: Needs work » Needs review
FileSize
7.43 KB

- async: false
- insert to BODY instead of HEAD

droplet’s picture

Status: Needs work » Needs review
FileSize
9.88 KB

Ouch, it mixed diff kinds of problems. try again

droplet’s picture

Status: Needs work » Needs review
FileSize
14.1 KB

One more. It should be one error left.

fail: [Other] Line 177 of core/modules/system/src/Tests/Ajax/FrameworkTest.php:
Page now has the system/drupal.system library.

I left it as it for now. Needs convert to JSTest to get a better result.

droplet’s picture

Status: Needs work » Needs review
FileSize
14.06 KB
droplet’s picture

Cool. Everything working as expected now.

FilterOptionsTest is a random fail:
https://www.drupal.org/node/2868880#comment-12064396

fail: [Other] Line 176 of core/modules/system/src/Tests/Ajax/FrameworkTest.php:

The data strcture is changed. I will fix it later.

We could tidy up test cases later, any feedback for the main code? Thanks!!!


...
+      $resource_commands[] = new AddJsCommand('head', $scripts_attributes, 'insertBefore');
...
+      $resource_commands[] = new AddJsCommand('body', $scripts_attributes, 'appendChild');

I wonder if we should limit $method arg to 2 options only?

Wim Leers’s picture

Ohhhh! Can't wait for @nod_ and @drupal to review this :) Thanks, @droplet! Exciting!

jansete’s picture

I think that we have the same problem for quickedit component using CDN.

#73 solve quickedit problem :)

droplet’s picture

SAME patch and reroll for ES6.

droplet’s picture

cancerian7’s picture

I am also getting this image loading issue, it only works If I click twice on the image button because the required js file loads on the first click on the image button (with image loading message) and on the 2nd click image dialog appears. I am not using CDN module. I have set sub-domain in my settings.php

$settings['file_public_base_url'] = '//s.example.com/files';

Any workable patch for this issue?

Thanks.

droplet’s picture

@cancerian7, try the same patch. CDN modules easier to reveal the problem only

GrandmaGlassesRopeMan’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Needs work » Needs review
FileSize
17.74 KB

- reroll from #80

aheimlich’s picture

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
@@ -174,11 +174,13 @@ protected function buildAttachmentsCommands(AjaxResponse $response, Request $req
+      $resource_commands[] = new AddJsCommand('head', $scripts_attributes, 'insertBefore');
+++ b/core/misc/ajax.es6.js
@@ -1337,5 +1350,36 @@
+            document.querySelector(selector)[response.method](scriptEl);

Node.insertBefore() has two parameters, both of which are required. You probably want to do something like this:

let parentEl = document.querySelector(selector);

if (response.method === 'insertBefore') {
  parentEl.insertBefore(scriptEl, parentEl.firstChild);
} else {
  parentEl[response.method](scriptEl);
}
droplet’s picture

Status: Needs work » Needs review
FileSize
18.15 KB

Random fails I think. All fails involved dialog like features and the loader makes the script loading slightly faster. I ran few failed tests manually and no problems.

Updated with #87. Good catch!

droplet’s picture

turn out the patch made the code asynchronously and the `ajaxComplete` executed before `ajaxSuccess` callbacks. (script files hadn't fully loaded yet)

Therefore, `setting.ajaxing` set to false and `waitOnAjaxRequest` return TRUE. That's why manually testing works but Test is failed.

Script execution flow:

Patch #89:
Click -> `ajaxSuccess` -> `ajaxComplete` -> `OpenDialog`

Expected:
Click -> `ajaxSuccess` -> `OpenDialog` -> `ajaxComplete`

droplet’s picture

One of possible workaround is to add another Promise to ajaxComplete

droplet’s picture

OK. QuickEdit has custom success callback

Expected 2 fails left

Ada Hernandez’s picture

GrandmaGlassesRopeMan’s picture

@Adita - We're not modifying the .js files anymore. Have a look at https://www.drupal.org/node/2815083 for more info.

Ada Hernandez’s picture

oh, ok sorry, I'll check it

droplet’s picture

FileSize
22.39 KB

haha. I couldn't explain why myself patch #96 fixed fails tests.

droplet’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
28.82 KB

Get the test 100% passed first. I could explain the code and update IS soon.

** I will keep the non-min-JS until the final patch, it helps to debug.

Needs to enhance exist tests: (I thought we should split it to separate issues [each point])
- Exist API allowed to add SAME JS files to the page. We should verify it added correctly.
- Ensure `ajax.ajaxing = false` set at right timing.

martin107’s picture

Just minor touches while reading along

1) Despite the name

assets/vendor/loadjs/loadjs.min.js

is not a minified file --- and should not be treated as such

2) We can lock down a parameter type with an array prefix.

3) Minor coding standard fix to a js comment.

Wim Leers’s picture

Wim Leers’s picture

@droplet It's unfortunate you didn't post interdiffs :( That'd have made it much easier to understand what you were changing.

#103.1: See what @droplet said in #102: I will keep the non-min-JS until the final patch, it helps to debug. :)

Review:

  1. +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,69 @@
    +  public function __construct($selector, array $scripts, $method = 'appendChild') {
    ...
    +      'method' => $this->method,
    

    As of #73, we have this. Why do we need it?

  2. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
    @@ -174,11 +174,13 @@ protected function buildAttachmentsCommands(AjaxResponse $response, Request $req
    +      $resource_commands[] = new AddJsCommand('head', $scripts_attributes, 'insertBefore');
    ...
    +      $resource_commands[] = new AddJsCommand('body', $scripts_attributes, 'appendChild');
    

    Ah we need it for this. But … actually this doesn't make any sense. The "header" and "footer" distinction only actually makes sense when sending a HTML response. Not when loading JS via AJAX.

    We can remove this extra complexity as far as I can tell. If we can't, let's document why we need this.

Also fixed my nits instead of putting them in the review.

droplet’s picture

The "header" and "footer" distinction only actually makes sense when sending a HTML response. Not when loading JS via AJAX.

Why? Because the ordering doesn't matter?

If so, I can think of one point that valid. On a page with ajax content loading, body part could be replaced.

Also fixed my nits instead of putting them in the review.

Thanks! I love this style, show your patch instead. It helps patch move forward faster :)

droplet’s picture

I'm going to replace loadjs with my implementation in #48 cause Ajax System always loaded the jQuery. I think we do not need an extra 3rd party. Thoughts?

Wim Leers’s picture

martin107’s picture

Assigned: Unassigned » martin107

I am going to take a run at the reroll...

I am here because in a contrib module ... I am getting stuck on this exact issue.

https://www.drupal.org/project/flag/issues/2842386#comment-12537174

The test in that issue is testing a custom AJAX command which dispatches events to which a mock third party module attaches event listeners.

So that constitutes an outline of the test that we should include here as proof the issue in core has gone away.

I will work on removing the "Needs test" tag after roroll.

martin107’s picture

Here is the reroll.

Just making notes for myself if this blow up...

Conflicts occurred in core/misc/ajax.es6.js - git blame showed the conflicts were triggered by this issue.

#2818825: Rename all JS files to *.es6.js and compile them

and

#2925064: [1/2] JS codestyle: no-restricted-syntax

1) drush site install proceeded without error.
2) Manual testing of forms make me think this is ok... testbot, as always will have the last word.

Aless86’s picture

Thanks for the reroll martin107, it works for me.

ttkaminski’s picture

FileSize
1.07 KB

Anyone planning to backport the patch to d7 anytime soon? This bug is affecting several of our d7 sites. I've implement a very quick and dirty hack that seems to work fine for me so far. The idea behind the hack is to detect when an ajax insert command is being performed and convert the src url of the script tags to relative urls. You need to specify in the whitelist the domains you want to convert to relative. I'll continue to use the hack until backport of the above is done.

JMOmandown’s picture

@mmartin107 - While the patch appears to have fixed the broader issue. There is still a lingering conflict from the solution dealing with the expected behavior of dialog ajax links.

<a type="button" class="btn-default use-ajax" data-dialog-type="modal" data-dialog-options="{&quot;width&quot;:800}" href="link_to_content">AJAX Modal</a>

The typical usage above fails even with the 'core/drupal.dialog.ajax' library attached to the parent page. This only occurs once the ajax page has been loaded. The link functionality in both the parent undelivered content and the new ajax delivered content is lost. I can confirm the dialog.ajax.js script has been retained / loaded from the parent page.

Wim Leers’s picture

keithdoyle9’s picture

FileSize
7.87 KB

We just ran into this problem yesterday as well when we switched over to using the CDN module for JS assets.

Drupal Version: 8.5.6
CDN Module: 8.x-3.2

Get the following error:
Error screen capture

ckeditor wouldn't load when you clicked "Edit" for a Paragraph in that node.

JMOmandown’s picture

This is also an issue when element calling ajax command was loaded via Ajax. Steps to reproduce child ajax issue:

  1. Attach core/drupal.ajax to both parent and below rendered
  2. Use AjaxResponse() to replace html with rendered content
  3. Attempt modal via "use-ajax" class
  4. Ajax Fails to Process and a normal redirect occurs
RumyanaRuseva’s picture

The last submitted patch, 113: 1988968-113.patch, failed testing. View results

jansete’s picture

Status: Needs work » Needs review
FileSize
29.37 KB

Patch rerolled

jansete’s picture

jansete’s picture

Status: Needs work » Needs review
FileSize
29.34 KB
jrockowitz’s picture

Webform has also run into this issue.
@see #2931295: Ajax does not work with BigPipe

The patch from #130 does fix the issue.

s.messaris’s picture

Patch #130 doesn't apply to 8.6.x and 8.7.x, because core/modules/system/src/Tests/Ajax/FrameworkTest.php and core/modules/system/src/Tests/Ajax/DialogTest.php have been removed. Here is a reroll of 130 without the changes on those files, although we probably should add testing somewhere else.

jansete’s picture

Current patch doesn't apply because class /core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
doesn't exist (now web tests are broswer tests https://www.drupal.org/node/2870458)

Attach the patch.

Phil Wolstenholme’s picture

Is anyone using this patch in production, or is far from being ready yet? I've found it hard to work out how close it is to being RTBC.

I've just tried it on one of our sites with this issue and there seems to be an issue with calling behaviours, I'm seeing one of my theme behaviours being executed before a contrib behaviour whose output the theme behaviour relies on. It might be unrelated, but prior to applying the patch the order was consistently contrib and then the theme.

My theme's behaviour rutherfordFacets had previously always been running after Facets's behaviour facetsCheckboxWidget, but after applying the patch rutherfordFacets runs before facetsCheckboxWidget.

(See #141 re: deletion above)

Wim Leers’s picture

#139: it's far from RTBC because there's little feedback, since most sites just don't run into this problem. So there's little interest from the community at large in fixing this, unfortunately. We'll need a few developers of sites who are actively running into this to collaborate: to test the patch in production, to provide test coverage, to build confidence that this is indeed solving the problem in the correct way.

Phil Wolstenholme’s picture

Thanks Wim.

Somewhat embarrassingly, the behaviour problems I mentioned in my last message were caused by a bug in Contrib, not this patch.

I'm not backend-y enough to produce tests, but we're testing this patch at the moment and hoping to deploy it to production relatively soon if it gets through QA.

Wim Leers’s picture

Don't be embarrassed — we all make mistakes :) Better to make cheap mistakes like that one — I've made much more expensive ones!

Phil Wolstenholme’s picture

I've noticed that with this patch applied, a single behaviour attached to multiple blocks that are loaded by BigPipe is only attached once, whereas without the patch they are attached once per block.

Here's what I'm seeing, and potential steps to reproduce:

I have a Facets Summary block, and then four Facets blocks, shown in a sidebar. I have BigPipe enabled. Each block has a Twig template which calls attach_library with a behaviour called rutherford/facets.

With the patch applied, rutherford/facets is only attached once, and when its attach() function is called the context being passed into it is the Facets Summary block. After that, rutherford/facets isn't attached any more times, despite the Facets block Twig template referencing it.

Without the patch applied, rutherford/facets is attached five times, with the context each time being either the one Facets Summary block, or one of the four Facets blocks.

With the patch applied, but with BigPipe disabled, then the behaviour is called once, but with document as the context, so it applies successfully to each of the five blocks.

Phil Wolstenholme’s picture

@phenaproxima's comment above has reminded me that I should report back.

We have been using a modified version of the patch in #137 in production for about two months now without identifying any issues.

I've attached the revised patch but unfortunately I can't remember what it was my colleague changed, I'll ask Rob and see if we can feedback on what we changed and if it would be relevant to other sites.

geek-merlin’s picture

Here's the trivial interdiff 137-145:

 $ git diff 1988968-137
diff --git a/core/assets/vendor/loadjs/loadjs.min.js b/core/assets/vendor/loadjs/loadjs.min.js
index bc398a6b08..6e1f5f799d 100644
--- a/core/assets/vendor/loadjs/loadjs.min.js
+++ b/core/assets/vendor/loadjs/loadjs.min.js
@@ -1,4 +1,4 @@
-loadjs = (function () {
+var loadjs = (function () {
   /**
    * Global dependencies.
 
geek-merlin’s picture

Eagerly awaiting this.

jefuri’s picture

This patch is awesome, using the latest. Works like a charm.

We had this issue when loading paragraphs with a CKEditor field. When the node itself did not have it already loaded because of the (removed) body field.
When we collapsed the paragraph, and reopened it the ckeditor workt again. Because then the necessary behavior for the editor was loaded.

Wim Leers’s picture

Pinged the JS maintainers @nod_, @alwaysworking and @justafish. It seems interest in fixing this is increasing :)

droplet’s picture

@jefuri,

have you loaded scripts from cross-origin? different domains, port, and subdomain are cross-origin.

If not, it could be another issue, e.g.: #2903614: Race condition results in same CSS+JS being loaded twice: race between BigPipe's server-side dynamic asset loading and Quick Edit's client-side dynamic asset loading

By theory, this thread issue should only happen in cross-origin env. Coincidentally the patch solved or reduce the error rate.

jefuri’s picture

Yes, it's cross origin. We have it running on google buckets with assets. The migration to the google buckets triggered the issue. So this is definitely related with this issue. And also, the latest patch resolved this.

martin107’s picture

Just looking at this for a moment.

from the patch in #145 and the library being pulled in

+  remote: https://github.com/muicss/loadjs
+  version: 3.5.1

we are 7 releases behind. and consequently our patch looks outdated the latest release is 3.6.1 ( released on on 11 Apr )

geek-merlin’s picture

It would be really helpful if someone who is into this patch comments on the related issue.

andriic’s picture

Version: 8.9.x-dev » 8.8.x-dev
Issue tags: -JavaScript +JavaScript
FileSize
23.94 KB

Check the updated patch #145 for version for v8.8.x.

There was a problem patching ajax.js

Martijn de Wit’s picture

Status: Needs work » Needs review
olli’s picture

Here is a re-roll of #145 for 8.8.x

olli’s picture

Status: Needs work » Needs review
FileSize
23.59 KB
2.82 KB

Fixed MessageCommandTest with Drupal.attachBehaviors() in add_js like it is/was done in insert command.

olli’s picture

Fixed Drupal\FunctionalJavascriptTests\Ajax\CommandsTest and Drupal\Tests\views_ui\FunctionalJavascript\ViewsListingTest by delaying the Drupal.attachBehaviors() call in Drupal.Ajax.prototype.success() until ajax commands are executed and Drupal\Tests\system\Functional\Ajax\FrameworkTest by replacing Prepend/AppendCommand with AddJsCommand. Also moved the ajaxDeferred.resolve() to the end of Drupal.Ajax.prototype.success().

pianomansam’s picture

I came here via #2961616: Autocomplete js not working after js aggregation. in an attempt to resolve the core Media Library JS libraries not being attached on the first load when using the s3fs module to store JS and when JS aggregation was turned on. Subsequent loads of the Media Library saw those JS libraries attached correctly, as did disabling JS aggregation. The patch in #162 resolved my issue.

agata.guc’s picture

Patch #162 is awesome !! It resolves problems with attaching aggregated js files when using s3fs module to store it.

jberube’s picture

I was having the issue where JS init callbacks weren't firing after AJAX content loaded. It happened on admin pages and on blocks loaded by BigPipe, and only happened when S3FS and aggregation were both turned on. The patch in #162 seems to have fixed the issue for me. Thank you.

clemens.tolboom’s picture

Guess this needs version 9.1.x and then a backport to 8.9.x? Anyone?

extect’s picture

Version: 8.8.x-dev » 9.1.x-dev
droplet’s picture

Should I release the custom module for it?

collecting usage:
https://forms.gle/SCPsPADHAxboW4tP6

acolden’s picture

Patch #162 fixed our issue.

Our issue was:
- we were using createInstance()->build() to build a custom block and insert into our homepage
- the block had a JS library attached (a basic JS slider)
- loading the homepage as anonymous DID work (CDN yes, BigPipe no)
- loading the hompage as signed in DID NOT work (CDN yes, BigPipe yes)
- disabling the CDN and loading the hompage as signed in DID work (CDN no, BigPipe yes)

We noticed that if the JS file was downloaded with BigPipe from the same server it was downloaded as an XHR but if it was downloaded from the CDN it was not. So if the file was not downloaded as an XHR then BigPipe didn't have the AJAX states to look at?

We could forcibly circumvent this particular issue by attaching the library to the hompeage, but patch #162 is definitely the proper fix!

extect’s picture

Tried to make #162 apply to 8.9.x

extect’s picture

FileSize
6.86 KB

Oh. Incomplete patch in #170. Here comes a new try to make #162 apply to 8.9.x

extect’s picture

FileSize
28.41 KB

Ouch. Sorry guys. Here we go again. Patch from #162, but for 8.9.x

extect’s picture

extect’s picture

Status: Needs review » Needs work

Did a lot of testing. Unfortunately, the patches in #162, #172 and #173 break core media library. The media selected in the library does not get included in the media reference field.

olli’s picture

Status: Needs work » Needs review

@extect Could you please provide steps to reproduce the problem with media library. I was able to reproduce it without patch #173 like this:

  1. install 9.1.x standard profile at localhost/drupal
  2. add $settings['file_public_base_url'] = 'http://127.0.0.1/drupal/sites/default/files'; to sites/default/settings.php
  3. enable media and media library modules
  4. add unlimited media reference field of external video to basic page
  5. edit basic page
  6. click "Add media", add two videos, select them, click "Insert selected"
  7. selected videos are not included

After applying #173 the selected videos are included.

nod_’s picture

updated the patch (reformated the JS) and clarified a few things. The loadjs lib is not the minified version, need to be updated.

To sum up what happened, essentially what the patch does

  • add an execution queue for drupal ajax commands that understands promises.
  • add an add_js command in JS that returns a promise. That promise is settled once all the files have been loaded (this is done with loadjs).

Few notes:

  • I don't think we should allow other script to mess with this queue, if there is something to do, do it in the php before it is sent to the browser. It's only a utility, not an extension point. So I didn't create the queue when the request is sent, only once the response is being processed.
  • I didn't want to change the ajax.ajaxing = false bit and add it to the execution queue because I wanted to separate the ajax request and the processing of the request. So we go to ajaxing false earlier than before, but the network part of the request is actually done, which is what this toggle is for ajax.ajaxing prevents the element from triggering a new request, the request is done, if another one is made it will be queued by the browser. I don't think it'll be an issue.
viappidu’s picture

Patch at #162 solved my problem on Drupal 8.8.x with Leaflet maps not being loaded on homepage. Gret job!

martin107’s picture

Trivial change, just correcting coding standard violations.

Wim Leers’s picture

#176: 😍😍😍😍😍 YAYYYYYYYYYYYYYYY!

@nod_: I think that if you could add a Nightwatch test, this'd be 100 times easier to land!

nod_’s picture

Rough tests, it's testing the execution order, not the JS loading.

droplet’s picture

Besides the removed code,
my patch #102 and nod_'s patch #176 has different logic on the main part.

Simplified code would look like this:

@droplet version: ensure all returns will be executed, no side effect, pure function

function outOfScoped() {
 return jQuery.Deferred().reject();
}

[1,2,3,4].reduce((deferredCommand, currentValue) => {
return deferredCommand.then(function () {
  console.log(currentValue)
  outOfScoped();
});
}, jQuery.Deferred().resolve().promise())

@nod_ version: incorrect/careless code will terminated the script, has side effect

function outOfScoped() {
 return jQuery.Deferred().reject();
}

[1,2,3,4].reduce((deferredCommand, currentValue) => {
return deferredCommand.then(function () {
  console.log(currentValue)
  return outOfScoped();
});
}, jQuery.Deferred().resolve().promise())

And Drupal.AjaxCommands may not return a Promise object, right?

nod_’s picture

If you don't return something from inside the .then() then the order is not guaranteed, because the JS will keep executing the callbacks without waiting on anything.

Try out the test code, if you remove the return, the version of the code in #102 will have the results out of order.
Ajax commands may not return an promise but that's not an issue, we don't use the return value in the successive functions,

Having incorect/careless code stop the execution seems reasonable to me, since we're making commands dependent on each other. It's clearer to fail at this step than continue execution and have an obscure error because a dependency has failed to load/execute.

You are right that we need some kind of error handling though, need to add a .catch() at the end.

If we use await, it might be easier to understand for everyone.

nod_’s picture

Added a catch, that log the error to the console.

I updated the loadjs lib and put the minified version, since it's now in version 4.2.0, still works as expected. (this is not in the interdiff, only the global patch).

droplet’s picture

I just reliased #102 also returned the value. It's 3 years long, haha.

How can I catch & revert the changes from the previous commands? Should we?

await version maybe more complicated.

By removing the ajaxing code. The this.executionQueue is unnecessary if I'm right.

nod_’s picture

FileSize
28.15 KB
3.39 KB

haha no worries :)

Yep, no need for an explicit variable. If we didn't need to support IE11 we could use native promise and get rid of the jQuery dependency.

I don't think we should try to recover, mainly because we can't do it now and I don't think we can ever do it reliably (because of atttachBehaviors and so on). It's more reasonable to fail and notify the user.

nicxvan’s picture

Applying the patch in #172 allows a library we attach to a webform via ajax to work. However we began to see some events fire twice, after investigation it seems we hit: https://www.drupal.org/project/drupal/issues/2903614

We had already disabled Quick edit previously due to issues with layout builder.
Disabling Big pipe fixes our issue of JS being added twice.

nod_’s picture

  1. +++ b/core/misc/ajax.es6.js
    @@ -1576,5 +1597,59 @@
    +            if (script.defer) {
    +              scriptEl.defer = true;
    +            }
    

    Defer is only useful before domcontentloaded, which even in the case of bigpipe the event has already been fired. Don't think we need to care about this one.

  2. +++ b/core/misc/ajax.es6.js
    @@ -1576,5 +1597,59 @@
    +            // To avoid synchronous XMLHttpRequest on the main thread and break
    +            // load dependency, it should not use jQuery.
    +            if (response.method === 'insertBefore') {
    +              parentEl.insertBefore(scriptEl, parentEl.firstChild);
    +            } else {
    +              parentEl[response.method](scriptEl);
    +            }
    +            // Return `false` to bypass loadjs' default DOM insertion mechanism.
    +            return false;
    

    Not caring about defer would allow us to use the loadjs default insertion.

bnjmnm’s picture

Status: Needs review » Needs work
  1. The quickedit and js_ajax_test .es6.js files need to haveyarn prettier run on them
  2. Loadjs needs the official dependency evaluation before it can be added. Looks like an excellent library.
  3. This may be specific to my local, but running yarn build:js on this patch results in a changed quickedit.js, although the diff does not appear to show an actual change?
    diff --git a/core/modules/quickedit/js/quickedit.js b/core/modules/quickedit/js/quickedit.js
    index 4e07d0a9cb..6a0fee6917 100644
    --- a/core/modules/quickedit/js/quickedit.js
    +++ b/core/modules/quickedit/js/quickedit.js
    @@ -383,4 +383,4 @@
           }
         }
       });
    -})(jQuery, _, Backbone, Drupal, drupalSettings, window.JSON, window.sessionStorage);
    +})(jQuery, _, Backbone, Drupal, drupalSettings, window.JSON, window.sessionStorage);
  1. +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,69 @@
    +   * @param array $scripts
    +   *   An array containing the attributes of the scripts to be added to the page.
    

    Would it be more accurate/helpgul to say it is "An array containing the attributes of the <script> tags to been added to the page."?

  2. +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,69 @@
    +   *   An array containing the attributes of the scripts to be added to the page.
    

    CS nit: one char too many for this line

  3. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
    @@ -174,11 +174,17 @@ protected function buildAttachmentsCommands(AjaxResponse $response, Request $req
    -      $resource_commands[] = new PrependCommand('head', $this->renderer->renderPlain($js_header_render_array));
    +      $scripts_attributes = array_map(function ($render_array) {
    +        return $render_array['#attributes'];
    +      }, $js_header_render_array);
    +      $resource_commands[] = new AddJsCommand('head', $scripts_attributes, 'insertBefore');
         }
         if ($js_assets_footer) {
           $js_footer_render_array = $this->jsCollectionRenderer->render($js_assets_footer);
    -      $resource_commands[] = new AppendCommand('body', $this->renderer->renderPlain($js_footer_render_array));
    +      $scripts_attributes = array_map(function ($render_array) {
    +        return $render_array['#attributes'];
    +      }, $js_footer_render_array);
    +      $resource_commands[] = new AddJsCommand('body', $scripts_attributes, 'appendChild');
    

    Previously it was pretty evident what was happening just by reading the line of code. These changes add enough additional logic that a comment would be helpful.

    It would also be nice if there's an elegant way to consolidate these two, since they're doing such similar things. It's worth trying out, but shouldn't be done if it hurts readability.

  4. +++ b/core/misc/ajax.es6.js
    @@ -992,53 +992,74 @@
    +              // If the commands returns a promise, allows to enforce the
    +              // execution order of the commands received.
    

    I don't quite understand this comment, but I think it'll be good with a tiny bit of rephrasing. It seems like it would be more appropriate above the then() vs. the return, but I may not fully understand.

  5. +++ b/core/misc/ajax.es6.js
    @@ -992,53 +992,74 @@
    +        // Uses jQuery deferred instead of native promises to support IE11.
    

    s/Uses/Use

  6. +++ b/core/misc/ajax.es6.js
    @@ -992,53 +992,74 @@
    +      // If the focus hasn't be changed by the ajax commands, try to refocus the
    +      // triggering element or one of its parents if that element does not exist
    +      // anymore.
    

    s/be/been
    This will be easier to read if split into two sentences
    "... try to refocus the triggering element. If that element does not exist anymore, try one of its parents."

  7. +++ b/core/misc/ajax.es6.js
    @@ -992,53 +992,74 @@
    +          !focusChanged &&
    +          this.element &&
    +          !$(this.element).data('disable-refocus')
    

    Is this focus logic covered by tests somewhere? (there may be, haven't investigated this too deeply) This seems like the kind of things that could inadvertently be broken by another patch. Doesn't necessarily need to be a dedicated test, an existing test that indirectly checks this would probably be fine too. Ah, I see this was not added in this patch, just moved. Keeping it here with strikeout since it's probably still worth noting.

  8. +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,69 @@
    + *
    
    +++ b/core/modules/quickedit/js/quickedit.es6.js
    @@ -178,12 +178,12 @@
    +    loadEditorsAjax.commands.add_js = function (ajax, response, status) {
    

    We have a func-name lint error, so this either needs to be arrowed or configed to skip if there's a need to use the function() scope.

  9. +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,69 @@
    +  protected $selector;
    

    I'd also like to know more about the decision to make $selector and $method individually available as opposed to an option to just choose 'head' and 'body'. There may be place where this flexibility is necessary, I just wanted to know a bit more.

  10. +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,69 @@
    +  public function __construct($selector, array $scripts, $method = 'appendChild') {
    

    Since $selector as NULL is valid, could this arg be moved and with a default value, it could simplify the calls to this the correct $method for NULL is already the default value for that variable.

  11. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
    @@ -174,11 +174,17 @@ protected function buildAttachmentsCommands(AjaxResponse $response, Request $req
    +      $resource_commands[] = new AddJsCommand('body', $scripts_attributes, 'appendChild');
    

    'appendChild' is already the default value for this arg, so it doesn't need to be specified here

nod_’s picture

Status: Needs work » Needs review
FileSize
27.48 KB
12.28 KB

1. done
2. todo
3. We can simplify the code actually, see interdiff.
1., 2. (bis) fixed
4. rewritten, hopefully clearer. Its to explain the meat of this patch.
5. fixed
6. The comment was there as-is before, didn't touch it.
8. see fix in interdiff (for 3.)
9/10/11. for better or worst, there is hardly any absctraction between Ajax commands and the DOM. head/body would be enough though, even then once we load files after the domcontentloaded, adding them to head or body shouldn't matter.
got rid of the method entierly and reordered the parameters of the command.

nod_’s picture

Status: Needs work » Needs review
FileSize
2.77 KB
27.26 KB

fix the test,

for 3. (bis) the answer was in the test code using array_column.

nod_’s picture

for library evaluation loadjs: https://github.com/muicss/loadjs

czigor’s picture

Just a reroll as the patch does not apply any more.

I'm unfamiliar with the core js dev workflow, followed https://www.drupal.org/docs/frontend-developer-tools-for-drupal-core#s-c... but there might be some unwanted changes.

nod_’s picture

Not sure what you did but git apply works with #193 :) are you applying the patch on the 9.1.x branch?

czigor’s picture

Ah ok, I missed that the patch was for d9. I was on 8.9.x. Nevermind then.

bnjmnm’s picture

Status: Needs review » Needs work

I like the addition of AddJsCommand, it is a good partner to AddCssCommand and makes the code easier to navigate.

The dependency evaluation will likely need a bit more information before it's committer-friendly, particularly since it's more than a dev dependency. An example of a recent non-dev dependency evaluation that was accepted can be found in the issue summary for #2550717: [JS] Replace jQuery.cookie with JS-cookie and provide a BC layer.

Since it doesn't look like a public security policy exists for loadjs, this will probably require opening an exploratory issue in Github, such as the one I did for js-cookie https://github.com/js-cookie/js-cookie/issues/614.

It may be good to include quick explanation about why the number open issues is not evidence of maintainership problems. Often it's just something like a maintainer less strict about closing issues requiring more information to proceed, and having that documented helps move things along.

Mostly nits ahead:

  1. +++ b/core/core.libraries.yml
    @@ -84,6 +85,16 @@ drupal.announce:
    +    url: https://github.com/muicss/loadjs/blob/master/LICENSE.txt
    

    Assuming it's available, it's preferred to link to the license in the release being used.

  2. +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,60 @@
    +   * page.
    

    CS nit, this initial desc needs to be single line.

  3. +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,60 @@
    +   *
    +   * If the command is a response to a request from an #ajax form element then
    +   * this value can be NULL.
    

    This looks technically accurate but it's a bit confusing since $selector has a default value of 'body' in the constructor, so $this->selector is always a string unless NULL was explicitly sent to the constructor, and even that looks like it would fall back to 'body' in the add_js JavaScript

  4. +++ b/core/misc/ajax.es6.js
    @@ -989,56 +989,83 @@
           .toArray();
     
    +
         // Track if any command is altering the focus so we can avoid changing the
    

    CS nit: extra line of whitespace

  5. +++ b/core/misc/ajax.es6.js
    @@ -989,56 +989,83 @@
    +                // When a commands returns a promise, the execution of the rest
    +                // of the commands will stop until this promise has been
    +                // fulfilled. Usually it is used to wait until the JavaScript
    +                // added by the 'add_js' command has loaded before continuing
    +                // the execution of the commands.
    

    This may benefit from reprhasing, something along the lines of

    // When a command returns a promise, the remaining comments
                    // will not execute until this promise has been fulfilled.
                    // Usually this is used to so commands will not proceed until
                    // until JavaScript assets added by the 'add_js' command have
                    // finished loading.
  6. +++ b/core/misc/ajax.es6.js
    @@ -989,56 +989,83 @@
    +        // anymore.
    

    The line length needs to change since moving it changed the indentation. 'be' could be changed to 'been' as well, although I'm aware this comment was just moved and not created in this patch.

  7. +++ b/core/misc/ajax.es6.js
    @@ -989,56 +989,83 @@
    +          console.error(
    

    I think using the console here is valuable, but it will require a // eslint-disable-next-line no-use-before-define before it.

  8. +++ b/core/misc/ajax.es6.js
    @@ -1576,5 +1603,55 @@
    +     */
    

    This needs a @return, a @param for response.selector could also be helpful.

  9. +++ b/core/misc/ajax.es6.js
    @@ -1576,5 +1603,55 @@
    +    add_js(ajax, response, status) {
    

    status unused

  10. +++ b/core/misc/ajax.es6.js
    @@ -1576,5 +1603,55 @@
    +          // All JS files were loaded and new and old behaviors have
    +          // been attached, resolve the promise and let the rest of the commands
    +          // execute.
    

    Nit: the 'been' can go up a line.

  11. +++ b/core/modules/system/tests/src/Functional/Ajax/FrameworkTest.php
    --- /dev/null
    +++ b/core/tests/Drupal/Nightwatch/Tests/ajaxExecutionOrderTest.js
    

    Looking through the test coverage, I see execution order is covered but I think it would be good to add a test that confirms loadjs both loads an asset and will not request it again if it is already present, and what the error behavior is if a requested resource does not exist.

Wim Leers’s picture

Exciting progress here! 🥳🤩

  1. +++ b/core/modules/quickedit/js/quickedit.es6.js
    @@ -178,16 +178,9 @@
    -    // Implement a scoped insert AJAX command: calls the callback after all AJAX
    -    // command functions have been executed (hence the deferred calling).
    -    const realInsert = Drupal.AjaxCommands.prototype.insert;
    -    loadEditorsAjax.commands.insert = function(ajax, response, status) {
    -      _.defer(callback);
    -      realInsert(ajax, response, status);
    -    };
         // Trigger the AJAX request, which will should return AJAX commands to
         // insert any missing attachments.
    -    loadEditorsAjax.execute();
    +    loadEditorsAjax.execute().then(callback);
    

    🤩 Nice clean-up!

  2. +++ b/core/modules/system/tests/modules/js_ajax_test/src/Ajax/JsAjaxTestCommandInsertPromise.php
    @@ -0,0 +1,27 @@
    +   * Implements Drupal\Core\Ajax\CommandInterface:render().
    

    🤓Shouldn't this be @inheritdoc?

  3. +++ b/core/modules/system/tests/src/Functional/Ajax/FrameworkTest.php
    --- /dev/null
    +++ b/core/tests/Drupal/Nightwatch/Tests/ajaxExecutionOrderTest.js
    

    😍🙏

acbramley’s picture

This bug surfaced in a slightly different way for me. I have a library attached via a field formatter on a block. When the block was added to Layout Builder for the first time none of the JS in the library was firing (this happens locally as well, no CDN at all). #193 fixed this for me :) Awesome work!

droplet’s picture

@acbramley,

- Is that something re-producible? Could you share the steps (which block you added also matter)

- If BigPipe enabled, try Comment #25 in #2903614: Race condition results in same CSS+JS being loaded twice: race between BigPipe's server-side dynamic asset loading and Quick Edit's client-side dynamic asset loading
(It could be a reason the current issue's patch reduce the performance a bit, lower probability.)

I have a quick look. Probably, it's part of BigPipe issue. Point #2 above should work I think.

https://git.drupalcode.org/project/drupal/-/blob/9.0.x/core/modules/layo...

It can also tell that we have buggy code on QuickEdit & LayoutBuilder. While this is async calling, we needed $.ready-alike thing checking (xxx.elementReady).

Wim Leers credited DuaelFr.

Wim Leers’s picture

@DuaelFr ran into this problem with the CDN module and reported a bug for it: #3164845: Serving JS from CDN prevent some behaviors to be attached.

I directed him here and he just reported at #3164845-4: Serving JS from CDN prevent some behaviors to be attached:

The patch in #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS fixes the issue. Thanks a lot!

DuaelFr’s picture

(I did write all this before Wim posted the previous comment, I don't want my so nice written english to be lost ^^)

I faced a similar issue using Media Library and the CDN module.
Simple steps to reproduce can be found in this issue: #3164845: Serving JS from CDN prevent some behaviors to be attached

I tested those steps with the patch from #193 and it fixes the issue.
I also added the patch from #195 on a live project running on Drupal 8.9.3 where an even more complicated combo of Paragraphs, Media Library and CDN were causing the issue, and it's fixed too.

Great job here, thanks a lot!

Wim Leers’s picture

@jefuri ran into this problem with the BigPipe Sessionless module and reported a bug for it: #3166441: BigPipe Sessionless does not execute drupalSettings or JS after streaming placeholders.

He just reported at #3166441-9: BigPipe Sessionless does not execute drupalSettings or JS after streaming placeholders that this patch indeed fixed their problem:

And darn it, it works. Updating the patch to the most recent one did it. I did thought this might be the case but thought it should work with the older version.

I thought I would have a mental breakdown, because I could not seem to figure out the issue.

(emphasis mine)

Wim Leers’s picture

Crediting @jefuri 😊

gapple’s picture

It looks like this intersects a bit with #3100151: Adding JS/CSS assets in AJAX responses requires 'unsafe-inline' Content Security Policy: Core's current method of rendering the asset markup server-side and appending it to the page requires 'unsafe-inline' in the page's Content Security Policy, whereas using createElement() client-side does not.

An issue though is that while conditional comments were removed from IE 10, core still supports them via the browsers attribute on library files #3095113: Deprecate IE conditional comments support (core removed any of it's own libraries using the attribute in 9.0).
The current patch doesn't consider this attribute, so would add contrib/custom library assets to all browsers, even if they were specified otherwise.

----

The add_css function was introduced to support IE <=9, which didn't fully support @import, but the special handing was since removed so it now it's just a wrapper for prepend #3100147: Remove @import parsing from Drupal.AjaxCommands.prototype.add_css #3110517: Improve Drupal\Core\Ajax\AddCssCommand to accept an array of CSS assets

----

I added the csp_extras submodule to the Content-Security-Policy project to override core's AJAX asset handling (#3166840: Override core's handling of library assets in AJAX responses).
It will skip any asset that is not set to all browsers with the browsers attribute and emit a warning. Both JS and CSS assets are added to the page by a single add_asset command.

It looks like the loadjs library also handles CSS by checking the file extension of the URL it was given, but needs a callback to set the media attribute.

das-peter’s picture

Also ran into this problem with the CDN module and found #3164845 with @Wim Leers pointer to this patch.
Patch from #195 applied fine to D8.9.7 and so far everything works just fine.

KapilV’s picture

Assigned: KapilV » Unassigned
Status: Needs work » Needs review
FileSize
21.79 KB
15.15 KB

Hear a reroll.

viappidu’s picture

Rewrite for 9.1.x

nod_’s picture

Please make sure to test your patches before sending them.

viappidu’s picture

Status: Needs review » Needs work
FileSize
27.24 KB

@nod_ you are absolutely right.
I only tested the patch for a clean installation on current 9.1.x, rewriting from patch at #211
After running the site I got t the error

Error: Class 'Drupal\Core\Ajax\AddJsCommand' not found in Drupal\Core\Ajax\AjaxResponseAttachmentsProcessor->buildAttachmentsCommands() (line 177...

making me realizing that @kapilkumar0324 did not add all the new files in your patch at #193.

Now corrected and tested on live site.

Not sure though how to work out comments by @gapple at #207
To me it would be natural to revert back to the previous prepend/append commands considered #3110517: Improve Drupal\Core\Ajax\AddCssCommand to accept an array of CSS assets though I simply cannot make it work that way with a result my scripts won't be added...
I think it still needs work

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
27.25 KB
ThirstySix’s picture

@bnjmnm Patch #218 works well on D9.x ( tried with 9.1.6)

matthiasm11’s picture

Wim Leers’s picture

I think the only thing remaining here is to get review from a JS maintainer such as @nod_?

SocialNicheGuru’s picture

Status: Needs review » Needs work

I added this to 8.9.13
I am using slick for a slideshow. It stopped showing up on the page. Google dev console showed it was being called but it was not being seen.

Views could not be edited.
The view would show butt when. you tried to edit any element, the popup did not show. Instead the edit modal became the full page.

EDIT:
This is for #195 and the 8.8+ patch

bnjmnm’s picture

Status: Needs work » Needs review

The feedback in #222 doesn't justify changing the status of this issue to "needs work" as the patch is for Drupal 9.2.x. The comment states that the problems occurred after adding to Drupal 8.9.13. Since the patch was not created for use with 8.x and there should be no expectation that it will work. The errors described sound pretty consistent with a JavaScript-including patch being applied to a version it was not intended for.

If problems are encountered with 9.2.x, absolutely report them here with steps to reproduce on a fresh Drupal install.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

danharper’s picture

The patch in #216 stops ckeditor working in layout builder. No errors in console, it just doesn't load the editor.

bnjmnm’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Adding "needs tests" tag as there should be a test to confirm CKEditor loads in off-canvas dialog, which would have caught #225. If I missed an existing test that does this, it may be due to that specific instance and the tag can be removed.

zrpnr’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
28.62 KB

I wasn't able to recreate the problem in #225, could you provide steps to recreate @danharper ?

From an umami install:
I went to a display managed by layout builder, admin/structure/types/manage/article/display/full/layout

  1. clicked "Add block"
  2. clicked "create custom block"
  3. clicked "basic block"

ckeditor loaded fine in the body field in the off-canvas.

Removing the "Needs tests" tag - for now - because this should be covered by testOffCanvasStyles
in core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php

Also worth noting the patch in #218 no longer applies to 9.2.x. Re-rolled for 9.3.x.

zrpnr’s picture

Addressed code quality checks

danharper’s picture

Sorry just checked again and it's working, not sure what I was doing before.

droplet’s picture

Caching (after the patch) or BigPipe or non-CORE code (that not using Drupal Ajax, or hacked DrupalAjax) may cause an error.

My suggestion after patching (, if you hit errors):

1. Clear Caching, clear a few times. Including the browsers
2. Disable BigPipe and try again (If that works, @see https://www.drupal.org/project/drupal/issues/2903614)
3. Open Console and check XHR requests

checklist for this bug report:

- [ ] Using CDN network or any cross-origin for JS (sub domains, www or non-www also counted)
- [ ] Clean CORE (no contrib modules. If you had, check XHR requests)
- [ ] Clear Caching
- [ ] Disable BigPipe

A valid bug should be all 4 ticks for the above list.

olli’s picture

  1. +++ b/core/assets/vendor/loadjs/loadjs.min.js
    @@ -0,0 +1 @@
    +loadjs=function(){var h=function(){},...
    

    Noticed that #145 added var here for some reason (see interdiff_137-145.txt in #158). Is it possible that a site could get a strict mode error?

  2. +++ b/core/modules/quickedit/js/quickedit.es6.js
    @@ -178,16 +178,9 @@
    +    loadEditorsAjax.execute().then(callback);
    

    It seems that now the "Quick edit" contextual link is visible before it is ready.

    Steps to reproduce (easier with throttling enabled):
    1. install standard 9.3.x
    2. create a basic page with some text in body field
    3. view the basic page and click "Quick edit" as soon as it is visible
    4. See error in console Uncaught TypeError: Drupal.quickedit.editors[editorName] is not a constructor

droplet’s picture

@olli,

can't reproduce #231

Only 2 requests on that page. History & QuickEdit. And the QuickEdit link comes/enabled from ajax. So that must be loaded.

John Pitcairn’s picture

Related: #3001543: TypeError: JS broken with BigPipe and js aggregation for logged-in users. Fixed by the patch at #216 for me in 9.1.x, not sure if there are any side effects yet.

DuaelFr’s picture

For those in need, here is the #228 patch rerolled for Drupal 9.2.x (applies on 9.2.0-beta2 and dev version so far).

viappidu’s picture

Refactored on lastest 9.3.x-dev

viappidu’s picture

FileSize
28 KB

Forgot to add new files in #235 patch.

Refactored 228 for 9.3.x

Martijn de Wit’s picture

@viappidu why are you refactoring? #228 is still working fine for 9.3.x right ?

viappidu’s picture

@Marttijn, at first I tried applying on my multi-patched installation then also on a plain 3.x (9.3.x-dev 4c4fc22) though I had a single rejection on a file, now don't ask me which one though, my terminal history doesn't go that far...
Haven't been working on my installation in a while, found the problem, tried to solve it. Mine it's working now :)

viappidu’s picture

@Martijn, you made me curious...
I retested #228 and found out actually two rejections

error: patch failed: core/.eslintrc.json:18
error: core/.eslintrc.json: patch does not apply
error: patch failed: core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php:110
error: core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php: patch does not apply

Difference in these files come from
Commit 9630f29
and
Commit 58fb7f8
I stand correct :)

Martijn de Wit’s picture

was still watching to the last valid test from 7 Jun 2021 at 18:10 CEST
so after that time, it failed indeed.

So you are completely correct :)

ThirstySix’s picture

I tried patch #236 with 9.2.0 version. it works well.
Also, I tried it with #234 works well.

pmagunia’s picture

Re-roll for 9.3

pmagunia’s picture

pmagunia’s picture

I'm going to try this again tonite.

I checked this patch - it does apply to 9.3.x.

I tried to fix the error "Custom Commands Failed." Not sure I got all the errors.

Also, I had trouble with the interdiff. The only thing I changed when creating the interdiff is I added a semi-colon to the end of the closing jQuery parenthesis to the patch in 228.

Oh I tried to run the sh script that the DrupalCI bot uses but the shell gives me errors, something about `[[ not found`. I guess because DrupalCI has to be installed on my dev environment.

Status: Needs review » Needs work

The last submitted patch, 244: 1988968-244.patch, failed testing. View results

xjm’s picture

Per discussion with @bnjmnm, I closed the old MR since it was created against 9.2.x and GitLab doesn't allow easily converting it to be an MR against 9.3.x. @bnjmnm will push a new branch built off 9.3.x for a new MR.

bnjmnm’s picture

Hey @pmagunia, I took the work you were doing in #242 - #244 and converted it to a merge request to simplify things. After doing so it occurred to me that I may have barged in on a task you were in the process of completing, and it would have been best to have pinged you about it first. Apologies for butting in on something you explicitly said you were going to be working on later in the day.

If you're interested in learning more about issue forks and merge requests see https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... and feel free to ping me on Drupal Slack if you have additional questions.

pmagunia’s picture

No worries @bnjmnm! I was getting stuck and didn't want to keep feeding test requests to DrupalCI. I'm glad you got the patch green!

nod_’s picture

Status: Needs work » Postponed
Wim Leers’s picture

Title: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS » [PP-2] Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS
nod_’s picture

Title: [PP-2] Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS » [PP-1] Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS

loadjs is in :)

Wim Leers’s picture

Yay! 🤩

Poindexterous’s picture

I'm running drupal 9.2.7. Patch #234 worked for me and helped resolve some js errors preventing the CKeditor from displaying in fields within the layout builder modal. However I tried #244 first and it seemed to break forms and left the following error in the log:
Error: Class 'Drupal\Core\Ajax\AddJsCommand' not found in Drupal\Core\Ajax\AjaxResponseAttachmentsProcessor->buildAttachmentsCommands() (line 181 of /mnt/www/html/jmidigitalstg/docroot/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php)

That was when I decided to try an earlier patch version.

nicxvan’s picture

I can second @poindexterous' experience. Using 234 seemed to resolve the AddJsCommand not found error.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

michael.barlow@mirumagency.com’s picture

FileSize
21.45 KB

Just reworked patch to work on released drupal 9.3.0

michael.barlow@mirumagency.com’s picture

michael.barlow@mirumagency.com’s picture

michael.barlow@mirumagency.com’s picture

bnjmnm’s picture

@michael.barlow@mirumagency.com - this issue switched to using a merge request instead of patches, and is working from 9.4.
The merge request has been rebased to apply to 9.4.

bartlangelaan made their first commit to this issue’s fork.

FiNeX’s picture

Hi, is it safe to apply patch #1988968-261: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS on a production website? Currently this bug prevent the site to use a CDN (if I want to let admins to correctly use the backend like adding images and links using ckeditor). Thank you.

Wim Leers’s picture

#264: Yes, it is. I would personally do so.

drupalninja99’s picture

We are getting this error now (see attached) after the core update.

clairemistry’s picture

I also got that error after 1) applying core update to 9.3.3 and then 2) applying patch #261
There was a missing closing } after applying the patch at the end of the AddJsCommand file (it was there in the actual patch file - but didn't apply for some reason).

Also in my case, which is a multistep ajax webform with an address lookup on the second step, the address lookup js was still not loaded after applying the patch. If I switch off ajax, the address lookup js is loaded and the lookup works as expected.

agata.guc’s picture

I fixed #261 patch. There was incorrect number of lines define in diff for AddJsCommand class.

joseph.olstad’s picture

Status: Postponed » Needs work
Issue tags: +Needs reroll

patch 268 of this issue works nicely in D9.2.16

Thanks so much , this resolves the following errors when using the clientside_validation module.

Uncaught TypeError: $.validator is undefined injectedScript:20:3 (from the clientside_validation module)

Patch 268 resolves this TypeError javascript issue wonderfully in 9.2.16. Looks like it needs a reroll for 9.3.x/9.4.x

yogeshmpawar’s picture

Updated patch with reroll diff.

Status: Needs review » Needs work

The last submitted patch, 270: 1988968-270.patch, failed testing. View results

cmlara’s picture

Issue summary: View changes

Updating IS to indicate that work should be done in #3228352: Add an add_js ajax command that uses promises

gapple’s picture

Issue summary: View changes

Updated issue summary

codebymikey’s picture

Issue summary: View changes

Updated issue summary including references to upstream jQuery issues as well as a snippet to demo the current issue.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

drupaluser2012’s picture

Hi,

The issue I am facing is created here https://www.drupal.org/forum/support/post-installation/2022-06-17/js-err...

Please let me know if anyone is facing the same issue and solved it using different patches given on this issue comments.

Thanks in advance.

joseph.olstad’s picture

I ended up backing off this patch and stopped using the clientside_validation module.

Maybe this would help
#3196973: Use Mutation observer for BigPipe replacements

azinck’s picture

@joseph.olstad: did you run into particular problems that made you stop using this patch?

drupaluser2012’s picture

@josepholstad

I'm not using 'clientside_validation' module. The error is visible even on /user/login page as well.

Thanks,

Megha_kundar’s picture

Tried fixing patch failed to apply issue

Status: Needs review » Needs work

The last submitted patch, 281: 1988968-281.patch, failed testing. View results

noorulshameera’s picture

nod_’s picture

Title: [PP-1] Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS » Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS
Status: Needs work » Needs review

Merging the two patch by committer request to make it possible to review.

Closing and porting credits from #3228352: Add an add_js ajax command that uses promises.

nod_’s picture

Status: Needs review » Needs work

The last submitted patch, 285: core-ajaxload-1988968-284.patch, failed testing. View results

nod_’s picture

FileSize
8.45 KB

Moved to native promise instead of jQuery deferred.

The issue was that ajax.ajaxing was set to false too soon (before all the commands have finished executing). This should resolve the failures.

nod_’s picture

Status: Needs work » Needs review
FileSize
27.16 KB
+++ b/core/misc/ajax.es6.js
@@ -554,10 +554,21 @@
+        return (
+          ajax
+            .success(response, status)
+            // Ajaxing status is back to false when all the ajax commands have
+            // finished executing.
+            .then(() => {
+              console.log('finished');
+              ajax.ajaxing = false;
+            })
+        );

This is the part that should fix the test failures.

Status: Needs review » Needs work

The last submitted patch, 288: core-ajaxload-1988968-287.patch, failed testing. View results

nod_’s picture

Status: Needs work » Needs review
FileSize
27.44 KB
2.14 KB

got a workaround for the media library thing. The media library code should be updated because it doesn't make sure add_js has finished before continuing to execute commands.

At least with this change we make sure we dont break legacy code.

No idea what's up with bigpipe. Test pass locally so that'll be tricky to find…

Status: Needs review » Needs work

The last submitted patch, 290: core-ajaxload-1988968-289.patch, failed testing. View results

nod_’s picture

Status: Needs work » Needs review
FileSize
28.14 KB
718 bytes

All right what I think is happening with bigpipe.

With the add_js command, adding the JS files doesn't stop script execution the same way it used to do by adding a bunch of html to the page.
Since adding those js files is non-blocking anymore, we can't rely on global variables declared in those files to be available right away.

In this case in the test the code is executed a bunch of times and the first time it's executed the JS files didn't have time to load yet hence the error. If we add the test for the CKEDITOR object things work as expected.

nod_’s picture

updated the media_library ui code to avoid too much duplication.

There are a bunch of contrib modules that overwrite the success method, https://git.drupalcode.org/search?group_id=2&scope=blobs&search=Drupal.A... so this will need a change notice. The code in core is safe enough to not crash is the overwrite is not complete. (bootstrap does it well, the others not so much).

nod_’s picture

added a change notice. Needs a bit of work to make the text better, it's a start.

nod_’s picture

FileSize
950 bytes

small tweak in the nightwatch test to use the drupalInstallModule command that's available.

nod_’s picture

nod_’s picture

What can be worked on after this one is #1014086: Stampedes and cold cache performance issues with css/js aggregation, this patch will prevent issues with that other patch.

nod_’s picture

let's not fix what's not broken. Test in #296 fails because of #3295477: Fix module search code on the extend page.

Reuploading the passing patch from #293.

nod_’s picture

Moved the test module code from the js_ajax_test module to the ajax_test module since the first one doesn't exists in D10 anymore. Was able to use the drupalinstallmodule command since ajax_test doesn't trigger the bug found in #298

Patch for 9.5 and for 10.x (with the dependency on promise polyfill removed).

nod_’s picture

with the linting, hitting the 300 comments mark :)

no interdiff, just a one line change in the nightwatch test.

nod_’s picture

Ajax test module has a confirmation step when installing, that's why the nightwatch test was failing.

here is hopping it's the last one :þ

nod_’s picture

Yes finally green, sorry for the noise.

Ready for review don't hesitate to ping me if you want me to walk you through the patch in slack/zoom/phone/letters/telegram

More than happy to do it if that makes it easier to rtbc and commit:)

larowlan’s picture

Issue tags: +Needs release note

Gave this a test on a project using Layout builder where we had a layout plugin that attaches Javascript (tabbed component). Just works™️

Here's a code review, but I'm really down to docs and nitpicks

  1. +++ b/core/misc/ajax.es6.js
    @@ -980,54 +1000,78 @@
    +          // Use jQuery deferred instead of native promises to support IE11.
    +          Promise.resolve(),
    

    Is this comment still accurate?

  2. +++ b/core/misc/ajax.es6.js
    @@ -1610,5 +1654,71 @@
    +            Object.keys(script).forEach((attributeKey) => {
    +              scriptEl.setAttribute(attributeKey, script[attributeKey]);
    +            });
    

    would Object.entries be cleaner here?

    Object.entries(script).forEach(([attributekey, attributeValue]) => scriptEl.setAttribute(attributeKey, attributeValue))
    
  3. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -83,37 +83,21 @@
    -            // Remove the progress element.
    -            if (this.progress.element) {
    -              $(this.progress.element).remove();
    -            }
    -            if (this.progress.object) {
    -              this.progress.object.stopMonitoring();
    -            }
    -            $(this.element).prop('disabled', false);
    -
    -            // Execute the AJAX commands.
    -            Object.keys(response || {}).forEach((i) => {
    -              if (response[i].command && this.commands[response[i].command]) {
    -                this.commands[response[i].command](this, response[i], status);
    +            return Promise.resolve(
    +              Drupal.Ajax.prototype.success.call(ajaxObject, response, status),
    

    We have a change record for this, but we should add a release note for any other instances where folks are messing about in the ajax internals and now need to return a promise.

  4. +++ b/core/modules/quickedit/js/quickedit.es6.js
    @@ -180,16 +180,9 @@
    -    // Implement a scoped insert AJAX command: calls the callback after all AJAX
    -    // command functions have been executed (hence the deferred calling).
    -    const realInsert = Drupal.AjaxCommands.prototype.insert;
    -    loadEditorsAjax.commands.insert = function (ajax, response, status) {
    -      _.defer(callback);
    -      realInsert(ajax, response, status);
    -    };
         // Trigger the AJAX request, which will should return AJAX commands to
         // insert any missing attachments.
    -    loadEditorsAjax.execute();
    +    loadEditorsAjax.execute().then(callback);
    

    this is so much nicer

nod_’s picture

1. Fixed
2. We still need IE support for the 9.x branch, so keeping as-is for now. If it ends up being a 10.x only patch, I'll update.
3. Added some text in the issue summary
4. :)

larowlan’s picture

Issue tags: -Needs release note

2. We still need IE support for the 9.x branch, so keeping as-is for now. If it ends up being a 10.x only patch, I'll update.

Ah, didn't know that wasn't available in IE, thanks

Thanks for the release note.

Is there anything we can do to detect if the success method doesn't return a promise and trigger a deprecation error then wrap it in a promise? I know its an edge case, but given we were doing it in core someone might be copying along from that example.

nod_’s picture

After your comment I thought about testing the success method for a promise return, I'm leaning towards not necessary.

In all case we will need to wrap the success method response in Promise.resolve() to make sure we can do the .then(() =>{ ajax.ajaxing = false }) in all cases, so even in D10 it wouldn't go away. And since the code wouldn't change between 9.x and 10.x I don't think triggering a deprecation is really what we want.

The real place this would be necessary is where people do things like what was done in the media library module, taking over the command execution in the ajax.success callback but we don't have a way to see if this is being taken over at runtime.

Looked at which modules were taking that over, seeing a few with low usage. So opening issues to those issues might be easier than trying to trigger a deprecation.

I thought about creating a new method in the Ajax object to wrap the command execution part so that people can reuse it more easily. But I'm not too happy about changing this monster patch at this point. I'll open a follow-up if that's something we want after this gets in.

nod_’s picture

umm yeah better to fix it now though. working on it.

nod_’s picture

9.5 version, the 10.x will be different.

Created a new helper function to create the execution queue for contrib reuse.
Added the deprecation message if the return of success is not a Promise.

nod_’s picture

for the 10.x code I'm throwing an error instead of just a deprecation message.

Status: Needs review » Needs work

The last submitted patch, 310: core-ajaxload-1988968-310-10.x.patch, failed testing. View results

gapple’s picture

Status: Needs work » Needs review

For 9.x, AddJsCommand has no awareness of the deprecated but not yet removed #browsers attribute on assets. Though conditional comments have not been supported past IE9, the current patch could introduce a change in behaviour for any sites that have not yet heeded the deprecation notice.

A possible option is to exclude any assets with ['#browsers']['!IE'] not set to TRUE

nod_’s picture

Started #3299002: Ajax promise execution queue 9.x and #3299003: Ajax promise execution queue 10.x. I'll continue the work there because this issue is way too long and slow to load.

nod_’s picture

tests failures are because the jQuery.form library override the success method and doesn't return a Promise. So we can't assume jquery plugins that interfaces with the ajax framework will update their code. Can't deprecate that unfortunately.

nod_’s picture

about #312 I don't have a good way of addressing that. This makes it a 10+ only patch then?

nod_’s picture

Actually for conditional comments I'm seeing them for CSS, not so much for JS even if it's technically possible. That would be an issue for a add_css command that uses loadjs but for JS we might get away with it, they can do browser detection inside of the JS.

Status: Needs review » Needs work

The last submitted patch, 310: core-ajaxload-1988968-310-10.x.patch, failed testing. View results

nod_’s picture

To recap,

  1. can't deprecate because success method can be used by jquery plugins such as jquery form and we can't expect them to change their code for our implementation detail.
  2. The fact that add_js doesn't support IE conditional comments is something to warn people about, but I don't think it's a deal breaker because on the JS side it's usually UA sniffing used inside of the code. Css can't do that so conditional comments are mostly for CSS (as we can see in core testing, we're only testing conditional comment in the context of css files, not JS)

I'm losing steam on this patch to be honest. I don't want to work much more on this one, I'd be happy to work on followups, but keeping this beast rerolled on 2 different branches is getting old.

nod_’s picture

Issue summary: View changes
rubens.arjr’s picture

Hi,

I applied the patch #318 and found that in some scenarios when aggregation of JS files is enabled the loadjs throws an error. This is a workaround for this scenario.

nod_’s picture

could you post the specific steps where the problem occurs and an interdiff where you hightlight the changes between your patch and the latest one? The solution you found might be the right one, it might also be a workaround hiding a deeper issue, without the scenario where this happens it's impossible to know which one is it.

This is a very long issue with a rather large patch so it's hard to follow if people just add changes without enough context. Thanks :)

rubens.arjr’s picture

FileSize
425 bytes

We have a page built with Site Studio that it's loaded with many JS files (such as slick carousel, accordions, back to top feature, and search autocomplete).
The "Aggregate JavaScript files" is enabled under the Performance configurations.
The error occurs only when anonymous users try to access the page, logged as admin the error doesn't occur. We noticed that the order of the JS files in the aggregated file changes depending if the user is anonymous or admin.

The errors being logged in the browser's console are: "Uncaught ReferenceError: assignment to undeclared variable loadjs" and "Uncaught ReferenceError: loadjs is not defined".

This error could also be fixed by adding a "var" or "let" at the begging of the minified loadjs file.

nod_’s picture

Sounds like a strict mode issue. It seems like one of your file in the anonymous case has a "use strict" directive at the top level, and since all the files are aggregated it applies to the whole aggregate, including loadjs, which is not "strict-safe" as it doesn't have a var/let/const declaration.

The weight change is a workaround for this (loadjs would be in the aggregate before your "use strict" code, rendering the "use strict" useless, or split in a different aggregate that doesn't have the "use strict") so I wouldn't include it in the patch because we're trying to remove the weights in libraries declarations: #1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering. I think you should try to find the script that declare "use strict" outside of a closure and wrap all that in a closure.

A possible followup would be to send a PR to loadjs so that a var is added to the variable declaration, in any case I don't think changing the weight in core is the right solution.

Patch to review is still #318

lauriii’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,60 @@
    + * An AJAX command for adding JS to the page via ajax.
    + *
    

    Can we add a little bit longer explanation of what this command does and what are the features of it?

  2. +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,60 @@
    +      'command' => 'add_js',
    

    I think the pattern we have established is to use camel case for command names

  3. +++ b/core/misc/ajax.es6.js
    @@ -547,10 +554,23 @@
    +      error(xmlhttprequest, status, error) {
             ajax.ajaxing = false;
    +      },
    +      complete(xmlhttprequest, status) {
    
    @@ -967,6 +1018,9 @@
    +   * @return {Promise}
    +   * The promise that will resolve once all commands have finished executing.
    

    The CR encourages to switching to use the Promise. Can we explain what's the benefit of doing that and what happens if they don't update their code?

  4. +++ b/core/misc/ajax.es6.js
    @@ -960,6 +980,37 @@
    +   *   Drupal Ajax response.
    
    @@ -1620,5 +1680,71 @@
    +            // with the assets that are not loaded with ajax.
    ...
    +      // Returns the promise so that the next AJAX command waits on the completion
    

    Nit: we have some minor inconsistencies where AJAX is sometimes all uppercase, sometimes all lowercase, and sometimes capital.

  5. +++ b/core/misc/ajax.es6.js
    @@ -1620,5 +1680,71 @@
    +      const allUniqueBundleIDs = response.data.map((script) => {
    ...
    +        const uniqueBundleID = script.src + ajax.instanceIndex;
    

    Nit: In camel case, this should be allUniqueBundleIds and allUniqueBundleId.

  6. +++ b/core/misc/ajax.es6.js
    @@ -1620,5 +1680,71 @@
    +          // By default, dynamically added scripts are marked as async. Only
    +          // explicitly marked async scripts should be loaded async.
    +          async: false,
    

    This comment is a bit confusing and doesn't read well. I guess it would be helpful to explain the relationship to what is being done inside the before callback.

  7. +++ b/core/misc/ajax.es6.js
    @@ -1620,5 +1680,71 @@
    +            // Return `false` to bypass loadjs' default DOM insertion mechanism.
    

    Nit: I don't think we need to wrap false with `

  8. +++ b/core/misc/ajax.es6.js
    @@ -1620,5 +1680,71 @@
    +          error(depsNotFound) {
    ...
    +              `The following files could not be loaded: @deps`,
    +              { '@deps': depsNotFound.join(', ') },
    

    Is deps a common abbreviation? It seems a bit unnecessary.

  9. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -83,37 +83,21 @@
    -            // Remove the progress element.
    -            if (this.progress.element) {
    -              $(this.progress.element).remove();
    -            }
    -            if (this.progress.object) {
    -              this.progress.object.stopMonitoring();
    -            }
    -            $(this.element).prop('disabled', false);
    -
    -            // Execute the AJAX commands.
    -            Object.keys(response || {}).forEach((i) => {
    -              if (response[i].command && this.commands[response[i].command]) {
    -                this.commands[response[i].command](this, response[i], status);
    +            return Promise.resolve(
    +              Drupal.Ajax.prototype.success.call(ajaxObject, response, status),
    +            ).then(() => {
    

    🤩

nod_’s picture

Assigned: Unassigned » nod_
borisson_’s picture

Assigned: nod_ » Unassigned

I'm very sorry, I have nitpicks.

  1. 
    ...
    +   * An array containing the attributes of the 'script' tags to be added to the
    +   * page.
    

    nitpick:

    Documentation should be like this:

    
    /**
     * one line max 80
     *
     * multiple other lines
     * go here.
     */
    

    So in this case, because of the one word being too long, this is not correct.

    I'd suggest rewriting to just An array containing the scripts to be added to the page.

  2. +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,60 @@
    +   * @param string|null $selector
    +   *   A CSS selector.
    

    I don't think this is correct, you can't assign NULL to the second variable, but it can be not filled in (in which case the value is defaulted to body).

    * @param string $selector
    *  (optional) A CSS Selector.
    
  3. +++ b/core/misc/ajax.es6.js
    @@ -537,10 +544,23 @@
    -      complete(xmlhttprequest, status) {
    +      error(xmlhttprequest, status, error) {
             ajax.ajaxing = false;
    +      },
    +      complete(xmlhttprequest, status) {
    

    We've split this up from having only complete to have complete and error handlers, but I think the bodies are the wrong way around? Here it seems like the error also does if status == 'error', which can/should go in the error part? Or am I misunderstanding?

    If I got this right, does that mean the ajax.ajaxing state is not tested correctly?

  4. +++ b/core/misc/ajax.es6.js
    @@ -979,55 +1033,61 @@
    +        // If the focus hasn't been changed by the ajax commands, try to refocus the
    +        // triggering element or one of its parents if that element does not exist
    

    80 cols

  5. +++ b/core/misc/ajax.es6.js
    @@ -979,55 +1033,61 @@
    +          // Reattach behaviors, if they were detached in beforeSerialize(). The
    +          // attachBehaviors() called on the new content from processing the response
    +          // commands is not sufficient, because behaviors from the entire form need
    ...
    +          // Remove any response-specific settings so they don't get used on the next
    +          // call by mistake.
    

    80 cols

  6. +++ b/core/misc/ajax.es6.js
    @@ -1610,5 +1670,71 @@
    +          // By default, dynamically added scripts are marked as async. Only
    +          // explicitly marked async scripts should be loaded async.
    +          async: false,
    

    there is a comment that mentions explicitly marked async scripts, but it is always set to false?

  7. +++ b/core/misc/ajax.es6.js
    @@ -1610,5 +1670,71 @@
    +      // Returns the promise so that the next AJAX command waits on the completion
    +      // of this one to execute, ensuring the JS is loaded before executing.
    ...
    +            // been attached, resolve the promise and let the rest of the commands
    

    80 cols

nod_’s picture

#326
1. fixed
2. fixed
3. I didn't want to add too many changes. In theory moving the code from the complete callback to the error callback is the same as running it without the IF. But I don't want to try my luck :)

Currently ajax.ajaxing is correct (except when adding JS files are involved) but having promise in the mix changes the execution order so we're not doing ajax.ajaxing = false in complete because the promise from the success callback can finish after the complete callback has been executed.

4. fix
5. fix
6. Yep, this is taken care of in the before callback, the setAttribute method would override that if necessary.
7. fix

#324
1. made an ajustement, but I'll need help if that's not enough :)
2. in the same file we have add_css, went with file consistency for the naming.
3. added some text in the CR
4. Think I got them all. Copy/pasted from add_css, so same problems there :)
5. fixed
6. tried to update the comment, help needed if that's not good enough
7. fixed
8. This is what loadjs calls them, replaced with @dependencies

I would love to handle nitpicks in followups, it's not straightforward to port changes from one branch to the other.

Status: Needs review » Needs work

The last submitted patch, 327: core-ajaxload-1988968-327-10.x.patch, failed testing. View results

nod_’s picture

Status: Needs work » Needs review
borisson_’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

The changes requested by @lauriii and me in previous comments have been resolved, and as far as I understand this implementing this will remove undefined behavior, so +1

phma’s picture

#327 seems to fix my issues with the media library widget like #3211232: Switching between media library tabs when JS aggregation is on and does not require me to exclude a dozen libraries from aggregation.
+1

gapple’s picture

Looks like #3188938: Create AjaxCommand for focusing that does not require :tabbable selector introduced some inconsistency by using camel case for focusFirst, but the older update_build_id and add_css commands use snake case.

bnjmnm’s picture

Status: Reviewed & tested by the community » Needs work

My feedback is all docs related, so if the changes are made by someone who has already engaged with this issue a decent amount, I think it's reasonable for them to self-rtbc when the feedback has been addressed. Suggested changes don't have to be implemented exactly, but I'd like them to address whatever clarity issues they attempted to resolve.

Code-wise this looks like a relatively straightforward solution and I'm very glad some of this was split into other issues so this was easier to review.

  1. +++ b/core/misc/ajax.es6.js
    @@ -960,6 +980,37 @@
    +            // the execution of the commands.
    

    Sugg:
    When a command returns a promise, the remaining commands will not execute until that promise has been fulfilled. This is typically used to ensure JavaScript files added via the 'add_js' command have loaded before subsequent commands execute.

  2. +++ b/core/misc/ajax.es6.js
    @@ -1620,5 +1680,73 @@
    +     *   A string that contains the JS files to be added.
    

    Is this accurate? Isn't is an array of objects of <script> attributes?

  3. +++ b/core/misc/ajax.es6.js
    @@ -1620,5 +1680,73 @@
    +        // loadjs requires a unique ID, AJAX instances' `instanceIndex` are
    +        // guaranteed to be unique.
    

    sugg:
    loadjs requires a unique ID, and an AJAX instance's `instanceIndex` is guaranteed to be unique.

  4. +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,60 @@
    +   * @param array $scripts
    +   *   An array containing the attributes of the 'script' tags to be added to
    +   *   the page.
    

    This @param description is accurate, but I think it may be difficult to understand if someone isn't already familiar with how it works. Since a multiline description is permitted, perhaps this could have a brief example like

    i.e. `['src' => 'someURL', 'defer' => TRUE]` becomes `<script src="someURL" defer></script>`
    </li>
    
    <li>
    <code>
    +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,60 @@
    +   *   A CSS selector.
    

    this should explain that this will be the element the script tags will be appended to.

  5. +++ b/core/misc/ajax.es6.js
    @@ -1610,5 +1670,73 @@
    +            // execute.
    

    Lets change the comma to a period to make this two sentences and bump 'commands' to a new line so it doesn't exceed 80 chars

  6. +++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
    @@ -0,0 +1,60 @@
    +   * this value can be NULL.
    

    I think this may have been addressed in the constructor already, but this can't ever be null since there's a default value that will be set if the arg isn't present or it's explicitly set to NULL.

  7. +++ b/core/misc/ajax.es6.js
    @@ -989,55 +1043,61 @@
    +          // call by mistake.
    

    The indentation change made this exceed 80 chars.

  8. +++ b/core/misc/ajax.es6.js
    @@ -1620,5 +1680,73 @@
    +            // By default, loadjs appends the script to the head. However, we
    +            // want to add the script to the parent specified. This is just for
    +            // consistency, because it doesn't actually matter for the script
    +            // where it is added. Developers however expect library assets to
    +            // show up where they declared them, so this makes things consistent
    +            // with the assets that are not loaded with AJAX.
    

    Sugg:
    By default, loadjs appends the script to the head. When scripts are loaded via AJAX, their location has no impact on functionality. But, since non-AJAX loaded scripts can choose their parent element, we provide that option here for the sake of consistency.

  9. +++ b/core/misc/ajax.es6.js
    @@ -1620,5 +1680,73 @@
    +            // All JS files were loaded and new and old behaviors have
    +            // been attached, resolve the promise and let the rest of the commands
    +            // execute.
    

    s/rest of the/remaining
    This will fix the 80 char limit as well.

ravi.shankar’s picture

Assigned: Unassigned » ravi.shankar

Working on this.

nod_’s picture

Sorry Ravi, it's a pretty annoying patch to update and I'm all set up for it already. It's already a long issue and I'd like to wrap it up as soon as possible. I'd like to avoid patch updates issues for documentation at this point.

lauriii’s picture

+++ b/core/lib/Drupal/Core/Ajax/AddJsCommand.php
@@ -37,9 +37,10 @@
+   *   `<script src="someURL" defer>.

Nit: missing closing `

nod_’s picture

arg :/ fix on commit? 🙏

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Confirmed that all feedback from #333 has been addressed.

bnjmnm’s picture

Issue summary: View changes

  • bnjmnm committed f976d5d on 10.1.x
    Issue #1988968 by nod_, droplet, bnjmnm, viappidu, extect, jansete, olli...

  • bnjmnm committed f33ee30 on 10.0.x
    Issue #1988968 by nod_, droplet, bnjmnm, viappidu, extect, jansete, olli...

  • bnjmnm committed c27ac21 on 9.5.x
    Issue #1988968 by nod_, droplet, bnjmnm, viappidu, extect, jansete, olli...
bnjmnm’s picture

Version: 9.5.x-dev » 9.4.x-dev
Issue tags: -DrupalWTF

This is a good solution, and I'm sure it would have already operated this way had the AJAX system been created a few years later when Promises were better supported. It's great to also great that the comments confirm that this will benefit many sites!

I've committed this to 9.5.x, 10.1.x (which cherry picked to 10.0.x). I'm holding off on 9.4.x as there might be a freeze on that branch, I'll check with the relevant folks to see if that's the case.

I'm also going to remove the DrupalWTF tag out of appreciation for the fact that core's ajax.js was added ~5 years before most browsers fully supported Promises. There's plenty of issues that warrant WTF, but in this case I think Drupal was just too ahead of it's time maaan.

lauriii’s picture

Version: 9.4.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

Given that this is also partly a feature addition since we're adding a new command, I would commit this for 9.5.x and up only. Marking as a fixed 👍

quietone’s picture

Issue tags: +10.0.0 release notes
nod_’s picture

Thanks everyone for all the help getting this done. Special mention to @droplet :)

Just realized we forgot to actually test that new add_js command, opened a follow-up: #3301769: Add test for the new add_js command

geek-merlin’s picture

🎉!

rubens.arjr’s picture

FileSize
26.86 KB

Made some changes based on #268 to get it working on 9.3.14.

ThirstySix’s picture

#338 works well with D9.4.5
Perfect👍

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

longwave’s picture

Issue tags: +9.5.0 release notes