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:
- https://stackoverflow.com/a/63296427
- https://github.com/jquery/jquery/issues/4801
- https://github.com/jquery/jquery/issues/1895
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
- #3228351: Add loadjs library
- #3228352: Add an add_js ajax command that uses promises
- Update
AjaxResponseAttachmentsProcessor
to useAddJsCommand
command (10.0.x, due to#browsers
)
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.
Related Issues
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.
Comment | File | Size | Author |
---|---|---|---|
#349 | 1988968-349-9.3.14.patch | 26.86 KB | rubens.arjr |
#338 | interdiff-335-338-10.x.txt | 586 bytes | nod_ |
#338 | core-ajaxload-1988968-338-10.x.patch | 34.07 KB | nod_ |
| |||
#338 | interdiff-335-338.txt | 586 bytes | nod_ |
#338 | core-ajaxload-1988968-338.patch | 35.62 KB | nod_ |
Issue fork drupal-1988968
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:
- 1988968-drupal.ajax-does-not changes, plain diff MR !1052
- 1988968-ajaxQueuer changes, plain diff MR !38
Comments
Comment #1
Wim LeersHrm… this seems odd.
So, what you're doing here, is essentially saying: "whenever
file_create_url()
executes on thesystem/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! :)
Comment #2
SpleshkaMay 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:
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.
Comment #3
Wim Leers#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
?Comment #4
SpleshkaI 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.
Comment #5
Wim LeersWell, 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?
Comment #6
SpleshkaI'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.
Comment #9
Wim LeersSorry for the delay — DrupalCon Portland happened…
I'll try to look at this tomorrow.
Comment #14
SpleshkaI 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.
Comment #15
Wim LeersThanks for the analysis!
I THINK your problem is this:
ensures files load fasteris apparently not a good one and makes those files take an even longer time to load.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 JSfasterslower, 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.
Comment #16
nod_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
Comment #17
SpleshkaOh, 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.
Comment #18
SpleshkaAny progress in this issue? I remember that someone tries to figure out what to do first here.
Comment #19
jessebeach CreditAttribution: jessebeach commentedIf 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 :)
Comment #20
SpleshkaI 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:
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.
Comment #21
nod_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.
Comment #22
SpleshkaOkey, I think we need some code to appear here :) I'm going to provide a quick patch to show how I see this.
Comment #23
gilgabar CreditAttribution: gilgabar commentedI'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.
And here is the simple js asset for it to load.
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.
Comment #24
nod_So basically here are the options on dynamic script loading: http://www.stevesouders.com/blog/2009/04/27/loading-scripts-without-bloc...
Comment #25
Spleshka@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 :)
Comment #26
nod_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.
Comment #28
cthshabel CreditAttribution: cthshabel commentedI 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?
Comment #31
Wim LeersThis 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: theopenDialog
AJAX command is called beforedialog.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 aadd_js
command (just like we already have aadd_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 theadd_js
AJAX command block on all JS files actually being loaded. That will then ensure that theopenDialog
AJAX command is called afterdialog.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 onDrupal.Ajax.prototype.success
being a blocking call.Comment #32
Wim LeersThis 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.
Comment #33
Wim Leers#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.
Comment #34
nod_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.
Comment #35
Wim LeersWe can even get there with just callback chaining AFAICT. Or what am I missing?
Comment #36
nod_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.
Comment #37
droplet CreditAttribution: droplet commentedHow about this way? A Promise
Comment #39
droplet CreditAttribution: droplet commentedFailed testing is expected while I added setTimeOut to simulate network delay. Let me upload one without it for testbots.
Comment #43
Wim LeersI worked on #2774507-7: Consider using Google's/YouTube's SPF JavaScript library a few days ago. That required me to use Google's/YouTube's https://github.com/youtube/spfjs, which also does tracking of loading of assets.
See https://github.com/youtube/spfjs/blob/v2.4.0/src/client/net/script.js + https://github.com/youtube/spfjs/blob/v2.4.0/src/client/net/resource.js for how they do this. https://github.com/youtube/spfjs/blob/v2.4.0/src/client/net/resource_tes... for test coverage.
Comment #44
Wim Leers@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?
Comment #45
Wim LeersI 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.Comment #46
droplet CreditAttribution: droplet commented@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.
Comment #47
Wim LeersInteresting. Sadly, it doesn't have a license yet. I left a comment at https://github.com/ded/script.js/issues/99.
Comment #48
droplet CreditAttribution: droplet commentedhaha. Just interesting the performance of my version above. Simplified a bit and use Script Tag.
https://jsfiddle.net/a2kj4j61/
Comment #49
Wim LeersThis is affecting people that are not even using a CDN: #2819001: CKEditor "image" dialog not loading anymore after updating to 8.2.1 [Solved].
Comment #50
heddnAnd also a while back (again with no CDN) in #2719083: Loading... ckeditor embed button
Comment #51
Wim LeersQuarterly bump.
Comment #52
droplet CreditAttribution: droplet commentednew rockstar:
https://github.com/muicss/loadjs
It's based on the $script.js and lightweight and MIT license.
Comment #53
Wim LeersOohh!
Comment #54
Wim LeersWow, 710 bytes! :O
Comment #56
droplet CreditAttribution: droplet commentedCan 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)
Comment #57
Wim LeersReported again at #2866693: Loading Focal Point module's JS as part of an AJAX call fails, this time when used with https://www.drupal.org/project/focal_point.
Comment #58
bojanz CreditAttribution: bojanz at Centarro commentedWe 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)
Comment #59
Wim LeersGlad to hear that in your case a work-around was possible. But it's not always an option…
Thanks for sharing/confirming!
Comment #60
Wim LeersReported again at #2873060: #states in forms break in modal dialog when states.js is loaded from CDN.
Comment #61
droplet CreditAttribution: droplet commentedtestbots?
Comment #63
droplet CreditAttribution: droplet commentedInteresting,
assets/vendor
is ignored in my local...Comment #65
droplet CreditAttribution: droplet commentedremove console.log
Comment #67
droplet CreditAttribution: droplet commented- async: false
- insert to BODY instead of HEAD
Comment #69
droplet CreditAttribution: droplet commentedOuch, it mixed diff kinds of problems. try again
Comment #71
droplet CreditAttribution: droplet commentedOne more. It should be one error left.
I left it as it for now. Needs convert to JSTest to get a better result.
Comment #73
droplet CreditAttribution: droplet commentedComment #75
droplet CreditAttribution: droplet commentedCool. Everything working as expected now.
FilterOptionsTest is a random fail:
https://www.drupal.org/node/2868880#comment-12064396
The data strcture is changed. I will fix it later.
We could tidy up test cases later, any feedback for the main code? Thanks!!!
I wonder if we should limit $method arg to 2 options only?
Comment #76
Wim LeersOhhhh! Can't wait for @nod_ and @drupal to review this :) Thanks, @droplet! Exciting!
Comment #77
jansete CreditAttribution: jansete commentedI think that we have the same problem for quickedit component using CDN.
#73 solve quickedit problem :)
Comment #78
droplet CreditAttribution: droplet commentedSAME patch and reroll for ES6.
Comment #80
droplet CreditAttribution: droplet commentedComment #82
cancerian7 CreditAttribution: cancerian7 commentedI 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.
Comment #83
droplet CreditAttribution: droplet commented@cancerian7, try the same patch. CDN modules easier to reveal the problem only
Comment #85
GrandmaGlassesRopeMan- reroll from #80
Comment #87
aheimlichNode.insertBefore()
has two parameters, both of which are required. You probably want to do something like this:Comment #89
droplet CreditAttribution: droplet commentedRandom 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!
Comment #91
droplet CreditAttribution: droplet commentedturn 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`
Comment #92
droplet CreditAttribution: droplet commentedOne of possible workaround is to add another Promise to ajaxComplete
Comment #94
droplet CreditAttribution: droplet commentedOK. QuickEdit has custom success callback
Expected 2 fails left
Comment #96
droplet CreditAttribution: droplet commentedFound an interesting bug:
#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
(new patch didn't fix any failed tests)
Comment #97
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedjust newline added at end of file (ajax.js)
Comment #98
GrandmaGlassesRopeMan@Adita - We're not modifying the
.js
files anymore. Have a look at https://www.drupal.org/node/2815083 for more info.Comment #100
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedoh, ok sorry, I'll check it
Comment #101
droplet CreditAttribution: droplet commentedhaha. I couldn't explain why myself patch #96 fixed fails tests.
Comment #102
droplet CreditAttribution: droplet commentedGet 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.
Comment #103
martin107 CreditAttribution: martin107 as a volunteer commentedJust 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.
Comment #104
Wim LeersPer #2903614-11: 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, this is blocking that issue.
Comment #105
Wim Leers@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:
:)Review:
As of #73, we have this. Why do we need it?
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.
Comment #106
droplet CreditAttribution: droplet commentedWhy? 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.
Thanks! I love this style, show your patch instead. It helps patch move forward faster :)
Comment #107
droplet CreditAttribution: droplet commentedI'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?
Comment #108
Wim LeersLooks like we have another issue reporting this problem: #2927687: Libraries with dependencies may cause race conditions with #lazy_builders.
Comment #112
martin107 CreditAttribution: martin107 as a volunteer commentedI 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.
Comment #113
martin107 CreditAttribution: martin107 as a volunteer commentedHere 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.
Comment #114
Aless86 CreditAttribution: Aless86 commentedThanks for the reroll martin107, it works for me.
Comment #115
ttkaminski CreditAttribution: ttkaminski commentedAnyone 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.
Comment #116
JMOmandown CreditAttribution: JMOmandown commented@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.
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.
Comment #118
Wim LeersIt looks like yet again somebody is running into this problem: #2976195: ckeditor.js is called before CKEDITOR library is loaded.
Comment #119
keithdoyle9 CreditAttribution: keithdoyle9 commentedWe 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:
ckeditor wouldn't load when you clicked "Edit" for a Paragraph in that node.
Comment #120
JMOmandown CreditAttribution: JMOmandown commentedThis is also an issue when element calling ajax command was loaded via Ajax. Steps to reproduce child ajax issue:
Comment #121
RumyanaRuseva CreditAttribution: RumyanaRuseva at FFW commentedPlease note that the patch conflicts with the one in #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs, and it will need to be re-rolled when the other issue is committed.
Comment #125
jansete CreditAttribution: jansete commentedPatch rerolled
Comment #127
jansete CreditAttribution: jansete commentedComment #130
jansete CreditAttribution: jansete commentedComment #132
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedWebform has also run into this issue.
@see #2931295: Ajax does not work with BigPipe
The patch from #130 does fix the issue.
Comment #133
s.messaris CreditAttribution: s.messaris commentedPatch #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.
Comment #137
jansete CreditAttribution: jansete commentedCurrent 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.
Comment #139
Phil Wolstenholme CreditAttribution: Phil Wolstenholme as a volunteer commentedIs 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 behaviourfacetsCheckboxWidget
, but after applying the patchrutherfordFacets
runs beforefacetsCheckboxWidget
.(See #141 re: deletion above)
Comment #140
Wim Leers#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.
Comment #141
Phil Wolstenholme CreditAttribution: Phil Wolstenholme as a volunteer commentedThanks 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.
Comment #142
Wim LeersDon't be embarrassed — we all make mistakes :) Better to make cheap mistakes like that one — I've made much more expensive ones!
Comment #143
Phil Wolstenholme CreditAttribution: Phil Wolstenholme as a volunteer commentedI'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 calledrutherford/facets
.With the patch applied,
rutherford/facets
is only attached once, and when itsattach()
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.
Comment #144
phenaproximaAdding #3061852: [META] Deprecate assertWaitOnAjaxRequest() and make the JsWebAssert::waitFor*() methods behave like real assertions as a related issue.
Comment #145
Phil Wolstenholme CreditAttribution: Phil Wolstenholme as a volunteer commented@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.
Comment #147
geek-merlinHere's the trivial interdiff 137-145:
Comment #148
geek-merlinEagerly awaiting this.
Comment #149
jefuri CreditAttribution: jefuri as a volunteer and at Synetic commentedThis 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.
Comment #150
Wim LeersPinged the JS maintainers @nod_, @alwaysworking and @justafish. It seems interest in fixing this is increasing :)
Comment #151
droplet CreditAttribution: droplet commented@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.
Comment #152
jefuri CreditAttribution: jefuri as a volunteer and at Synetic commentedYes, 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.
Comment #153
martin107 CreditAttribution: martin107 as a volunteer commentedJust looking at this for a moment.
from the patch in #145 and the library being pulled in
we are 7 releases behind. and consequently our patch looks outdated the latest release is 3.6.1 ( released on on 11 Apr )
Comment #154
geek-merlinIt would be really helpful if someone who is into this patch comments on the related issue.
Comment #156
andriic CreditAttribution: andriic commentedCheck the updated patch #145 for version for v8.8.x.
There was a problem patching ajax.js
Comment #157
Martijn de WitComment #158
olli CreditAttribution: olli commentedHere is a re-roll of #145 for 8.8.x
Comment #160
olli CreditAttribution: olli commentedFixed MessageCommandTest with Drupal.attachBehaviors() in add_js like it is/was done in insert command.
Comment #162
olli CreditAttribution: olli commentedFixed 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().
Comment #163
pianomansam CreditAttribution: pianomansam commentedI 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.
Comment #164
agata.guc CreditAttribution: agata.guc commentedPatch #162 is awesome !! It resolves problems with attaching aggregated js files when using s3fs module to store it.
Comment #165
jberube CreditAttribution: jberube commentedI 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.
Comment #166
clemens.tolboomGuess this needs version 9.1.x and then a backport to 8.9.x? Anyone?
Comment #167
extect CreditAttribution: extect commentedComment #168
droplet CreditAttribution: droplet commentedShould I release the custom module for it?
collecting usage:
https://forms.gle/SCPsPADHAxboW4tP6
Comment #169
acolden CreditAttribution: acolden commentedPatch #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!
Comment #170
extect CreditAttribution: extect commentedTried to make #162 apply to 8.9.x
Comment #171
extect CreditAttribution: extect commentedOh. Incomplete patch in #170. Here comes a new try to make #162 apply to 8.9.x
Comment #172
extect CreditAttribution: extect commentedOuch. Sorry guys. Here we go again. Patch from #162, but for 8.9.x
Comment #173
extect CreditAttribution: extect commented... and for 9.x
Comment #174
extect CreditAttribution: extect commentedDid 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.
Comment #175
olli CreditAttribution: olli commented@extect Could you please provide steps to reproduce the problem with media library. I was able to reproduce it without patch #173 like this:
$settings['file_public_base_url'] = 'http://127.0.0.1/drupal/sites/default/files';
tosites/default/settings.php
After applying #173 the selected videos are included.
Comment #176
nod_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_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:
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 forajax.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.Comment #177
viappidu CreditAttribution: viappidu as a volunteer commentedPatch at #162 solved my problem on Drupal 8.8.x with Leaflet maps not being loaded on homepage. Gret job!
Comment #178
martin107 CreditAttribution: martin107 as a volunteer commentedTrivial change, just correcting coding standard violations.
Comment #179
Wim Leers#176: 😍😍😍😍😍 YAYYYYYYYYYYYYYYY!
@nod_: I think that if you could add a
Nightwatch
test, this'd be 100 times easier to land!Comment #180
nod_Rough tests, it's testing the execution order, not the JS loading.
Comment #181
droplet CreditAttribution: droplet commentedBesides 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
@nod_ version: incorrect/careless code will terminated the script, has side effect
And
Drupal.AjaxCommands
may not return aPromise
object, right?Comment #182
nod_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.Comment #183
nod_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).
Comment #184
droplet CreditAttribution: droplet commentedI 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. Thethis.executionQueue
is unnecessary if I'm right.Comment #185
nod_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.
Comment #188
nicxvan CreditAttribution: nicxvan commentedApplying 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.
Comment #189
nod_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.
Not caring about defer would allow us to use the loadjs default insertion.
Comment #190
bnjmnmyarn prettier
run on themyarn build:js
on this patch results in a changed quickedit.js, although the diff does not appear to show an actual change?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."?CS nit: one char too many for this line
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.
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.s/Uses/Use
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."
I
s 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.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 thefunction()
scope.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.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.'appendChild' is already the default value for this arg, so it doesn't need to be specified here
Comment #191
nod_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.
Comment #193
nod_fix the test,
for 3. (bis) the answer was in the test code using array_column.
Comment #194
nod_for library evaluation loadjs: https://github.com/muicss/loadjs
Comment #195
czigor CreditAttribution: czigor at Centarro commentedJust 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.
Comment #196
nod_Not sure what you did but git apply works with #193 :) are you applying the patch on the 9.1.x branch?
Comment #197
czigor CreditAttribution: czigor at Centarro commentedAh ok, I missed that the patch was for d9. I was on 8.9.x. Nevermind then.
Comment #198
bnjmnmI like the addition of
AddJsCommand
, it is a good partner toAddCssCommand
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:
Assuming it's available, it's preferred to link to the license in the release being used.
CS nit, this initial desc needs to be single line.
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
CS nit: extra line of whitespace
This may benefit from reprhasing, something along the lines of
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.
I think using the console here is valuable, but it will require a
// eslint-disable-next-line no-use-before-define
before it.This needs a
@return
, a@param
forresponse.selector
could also be helpful.status unused
Nit: the 'been' can go up a line.
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.
Comment #199
Wim LeersExciting progress here! 🥳🤩
🤩 Nice clean-up!
🤓Shouldn't this be
@inheritdoc
?😍🙏
Comment #200
acbramley CreditAttribution: acbramley at PreviousNext commentedThis 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!
Comment #201
droplet CreditAttribution: droplet commented@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).
Comment #203
Wim Leers@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:
Comment #204
DuaelFr(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!
Comment #205
Wim Leers@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:
(emphasis mine)
Comment #206
Wim LeersCrediting @jefuri 😊
Comment #207
gappleIt 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 usingcreateElement()
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 forprepend
#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 singleadd_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.
Comment #209
das-peter CreditAttribution: das-peter at Cando commentedAlso 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.
Comment #211
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedHear a reroll.
Comment #213
viappidu CreditAttribution: viappidu as a volunteer commentedRewrite for 9.1.x
Comment #215
nod_Please make sure to test your patches before sending them.
Comment #216
viappidu CreditAttribution: viappidu as a volunteer commented@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
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
Comment #218
bnjmnmComment #219
ThirstySix CreditAttribution: ThirstySix as a volunteer and commented@bnjmnm Patch #218 works well on D9.x ( tried with 9.1.6)
Comment #220
matthiasm11 CreditAttribution: matthiasm11 at MM-Experience commented+1 for patch #218 on D9.1.6. This fixed https://www.drupal.org/project/leaflet/issues/3179158
Comment #221
Wim LeersI think the only thing remaining here is to get review from a JS maintainer such as @nod_?
Comment #222
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedI 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
Comment #223
bnjmnmThe 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.
Comment #225
danharper CreditAttribution: danharper as a volunteer and at hrpr commentedThe patch in #216 stops ckeditor working in layout builder. No errors in console, it just doesn't load the editor.
Comment #226
bnjmnmAdding "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.
Comment #227
zrpnrI 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
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.
Comment #228
zrpnrAddressed code quality checks
Comment #229
danharper CreditAttribution: danharper as a volunteer and at hrpr commentedSorry just checked again and it's working, not sure what I was doing before.
Comment #230
droplet CreditAttribution: droplet commentedCaching (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.
Comment #231
olli CreditAttribution: olli commentedNoticed 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?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
Comment #232
droplet CreditAttribution: droplet commented@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.
Comment #233
John Pitcairn CreditAttribution: John Pitcairn commentedRelated: #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.
Comment #234
DuaelFrFor 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).
Comment #235
viappidu CreditAttribution: viappidu as a volunteer commentedRefactored on lastest 9.3.x-dev
Comment #236
viappidu CreditAttribution: viappidu as a volunteer commentedForgot to add new files in #235 patch.
Refactored 228 for 9.3.x
Comment #237
Martijn de Wit@viappidu why are you refactoring? #228 is still working fine for 9.3.x right ?
Comment #238
viappidu CreditAttribution: viappidu as a volunteer commented@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 :)
Comment #239
viappidu CreditAttribution: viappidu as a volunteer commented@Martijn, you made me curious...
I retested #228 and found out actually two rejections
Difference in these files come from
Commit 9630f29
and
Commit 58fb7f8
I stand correct :)
Comment #240
Martijn de Witwas 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 :)
Comment #241
ThirstySix CreditAttribution: ThirstySix as a volunteer and at GTECH commentedI tried patch #236 with 9.2.0 version. it works well.
Also, I tried it with #234 works well.
Comment #242
pmaguniaRe-roll for 9.3
Comment #243
pmaguniaComment #244
pmaguniaI'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.
Comment #248
xjmPer 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.
Comment #249
bnjmnmHey @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.
Comment #250
pmaguniaNo 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!
Comment #251
nod_split things up to make it easier to review/commit:
#3228351: Add loadjs library
#3228352: Add an add_js ajax command that uses promises
Comment #252
Wim LeersComment #253
nod_loadjs is in :)
Comment #254
Wim LeersYay! 🤩
Comment #255
Poindexterous CreditAttribution: Poindexterous commentedI'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.
Comment #256
nicxvan CreditAttribution: nicxvan commentedI can second @poindexterous' experience. Using 234 seemed to resolve the AddJsCommand not found error.
Comment #258
michael.barlow@mirumagency.com CreditAttribution: michael.barlow@mirumagency.com commentedJust reworked patch to work on released drupal 9.3.0
Comment #259
michael.barlow@mirumagency.com CreditAttribution: michael.barlow@mirumagency.com commentedDisregard the previous patch, I forgot to include AddJsCommand class file.
Comment #260
michael.barlow@mirumagency.com CreditAttribution: michael.barlow@mirumagency.com commentedComment #261
michael.barlow@mirumagency.com CreditAttribution: michael.barlow@mirumagency.com commentedFixed linter issue
Comment #262
bnjmnm@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.
Comment #264
FiNeX CreditAttribution: FiNeX as a volunteer commentedHi, 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.
Comment #265
Wim Leers#264: Yes, it is. I would personally do so.
Comment #266
drupalninja99 CreditAttribution: drupalninja99 at Mediacurrent commentedWe are getting this error now (see attached) after the core update.
Comment #267
clairemistry CreditAttribution: clairemistry commentedI 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.
Comment #268
agata.guc CreditAttribution: agata.guc commentedI fixed #261 patch. There was incorrect number of lines define in diff for AddJsCommand class.
Comment #269
joseph.olstadpatch 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
Comment #270
yogeshmpawarUpdated patch with reroll diff.
Comment #272
cmlaraUpdating IS to indicate that work should be done in #3228352: Add an add_js ajax command that uses promises
Comment #273
gappleUpdated issue summary
Comment #274
codebymikey CreditAttribution: codebymikey at Zodiac Media commentedUpdated issue summary including references to upstream jQuery issues as well as a snippet to demo the current issue.
Comment #276
drupaluser2012 CreditAttribution: drupaluser2012 at Diamond TechSoft commentedHi,
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.
Comment #277
joseph.olstadI ended up backing off this patch and stopped using the clientside_validation module.
Maybe this would help
#3196973: Use Mutation observer for BigPipe replacements
Comment #278
azinck CreditAttribution: azinck at Forum One commented@joseph.olstad: did you run into particular problems that made you stop using this patch?
Comment #279
drupaluser2012 CreditAttribution: drupaluser2012 at Diamond TechSoft commented@josepholstad
I'm not using 'clientside_validation' module. The error is visible even on /user/login page as well.
Thanks,
Comment #280
dawid_nawrot CreditAttribution: dawid_nawrot as a volunteer commentedUpdate patch - works with Drupal 9.4.1
Comment #281
Megha_kundar CreditAttribution: Megha_kundar at Srijan | A Material+ Company for Drupal Association commentedTried fixing patch failed to apply issue
Comment #283
noorulshameera CreditAttribution: noorulshameera as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #284
nod_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.
Comment #285
nod_Comment #287
nod_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.Comment #288
nod_This is the part that should fix the test failures.
Comment #290
nod_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…
Comment #292
nod_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.Comment #293
nod_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).
Comment #294
nod_added a change notice. Needs a bit of work to make the text better, it's a start.
Comment #295
nod_small tweak in the nightwatch test to use the drupalInstallModule command that's available.
Comment #296
nod_Comment #297
nod_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.
Comment #298
nod_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.
Comment #299
nod_Moved the test module code from the
js_ajax_test
module to theajax_test
module since the first one doesn't exists in D10 anymore. Was able to use the drupalinstallmodule command sinceajax_test
doesn't trigger the bug found in #298Patch for 9.5 and for 10.x (with the dependency on promise polyfill removed).
Comment #300
nod_with the linting, hitting the 300 comments mark :)
no interdiff, just a one line change in the nightwatch test.
Comment #301
nod_Comment #302
nod_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 :þ
Comment #303
nod_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:)
Comment #304
larowlanGave 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
Is this comment still accurate?
would Object.entries be cleaner here?
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.
this is so much nicer
Comment #305
nod_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. :)
Comment #306
larowlanAh, 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.
Comment #307
nod_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.
Comment #308
nod_umm yeah better to fix it now though. working on it.
Comment #309
nod_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.
Comment #310
nod_for the 10.x code I'm throwing an error instead of just a deprecation message.
Comment #312
gappleFor 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 toTRUE
Comment #313
nod_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.
Comment #314
nod_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.
Comment #315
nod_about #312 I don't have a good way of addressing that. This makes it a 10+ only patch then?
Comment #316
nod_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.
Comment #318
nod_To recap,
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.
Comment #319
nod_Comment #320
rubens.arjr CreditAttribution: rubens.arjr at CI&T commentedHi,
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.
Comment #321
nod_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 :)
Comment #322
rubens.arjr CreditAttribution: rubens.arjr at CI&T commentedWe 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.
Comment #323
nod_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
Comment #324
lauriiiCan we add a little bit longer explanation of what this command does and what are the features of it?
I think the pattern we have established is to use camel case for command names
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?
Nit: we have some minor inconsistencies where AJAX is sometimes all uppercase, sometimes all lowercase, and sometimes capital.
Nit: In camel case, this should be
allUniqueBundleIds
andallUniqueBundleId
.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.Nit: I don't think we need to wrap false with `
Is deps a common abbreviation? It seems a bit unnecessary.
🤩
Comment #325
nod_Comment #326
borisson_I'm very sorry, I have nitpicks.
nitpick:
Documentation should be like this:
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.
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).
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?
80 cols
80 cols
there is a comment that mentions explicitly marked async scripts, but it is always set to false?
80 cols
Comment #327
nod_#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.
Comment #329
nod_Comment #330
borisson_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
Comment #331
phma CreditAttribution: phma at OM commented#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
Comment #332
gappleLooks like #3188938: Create AjaxCommand for focusing that does not require :tabbable selector introduced some inconsistency by using camel case for
focusFirst
, but the olderupdate_build_id
andadd_css
commands use snake case.Comment #333
bnjmnmMy 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.
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.
Is this accurate? Isn't is an array of objects of
<script>
attributes?sugg:
loadjs requires a unique ID, and an AJAX instance's `instanceIndex` is guaranteed to be unique.
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
this should explain that this will be the element the script tags will be appended to.
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
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.
The indentation change made this exceed 80 chars.
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.
s/rest of the/remaining
This will fix the 80 char limit as well.
Comment #334
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedWorking on this.
Comment #335
nod_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.
Comment #336
lauriiiNit: missing closing `
Comment #337
nod_arg :/ fix on commit? 🙏
Comment #338
nod_Comment #339
lauriiiConfirmed that all feedback from #333 has been addressed.
Comment #340
bnjmnmComment #344
bnjmnmThis 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.
Comment #345
lauriiiGiven 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 👍
Comment #346
quietone CreditAttribution: quietone at PreviousNext commentedComment #347
nod_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
Comment #348
geek-merlin🎉!
Comment #349
rubens.arjr CreditAttribution: rubens.arjr at CI&T commentedMade some changes based on #268 to get it working on 9.3.14.
Comment #350
ThirstySix CreditAttribution: ThirstySix as a volunteer and at GTECH commented#338 works well with D9.4.5
Perfect👍
Comment #352
longwave