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.

See #14 and #15 for details.

Proposed resolution

TBD.

Remaining tasks

TBD.

User interface changes

None.

API changes

TBD

TBD

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.

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.

Spleshka’s picture

Any updates here? Your sandbox is still waiting for you.

kstrasser’s picture

I am having this same problem...hoping for a fix soon.

Wim Leers’s picture

Sorry for the delay — DrupalCon Portland happened…

I'll try to look at this tomorrow.

Spleshka’s picture

Great, looking forward for your report.

Spleshka’s picture

@Wim Leers,

Did you reproduce this on my sandbox?

Spleshka’s picture

Any progress here?

Spleshka’s picture

Assigned: Wim Leers » Spleshka
Status: Postponed (maintainer needs more info) » Active

Going to find a problem myself in the nearest future.

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

Project: CDN » Drupal core
Issue summary: View changes

Updated issue summary.

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.

Spleshka’s picture

Issue summary: View changes

Updated issue summary.

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.

mgifford’s picture

Assigned: Spleshka » Unassigned
Issue summary: View changes
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?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Status: Needs review » Needs work

The last submitted patch, 37: promisedAjaxLoader.patch, failed testing.

droplet’s picture

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

droplet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 39: promisedAjaxLoader-1988968-39.patch, failed testing.

Wim Leers’s picture

Issue tags: +needs backport to D7
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: Known core bug: 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

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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?

Status: Needs review » Needs work

The last submitted patch, 61: 1988968-loadjs.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
8.32 KB

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

Status: Needs review » Needs work

The last submitted patch, 63: 1988968-loadjs-62.patch, failed testing.

droplet’s picture

remove console.log

Status: Needs review » Needs work

The last submitted patch, 65: 1988968-loadjs-65.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
7.43 KB

- async: false
- insert to BODY instead of HEAD

Status: Needs review » Needs work

The last submitted patch, 67: 1988968-67.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
9.88 KB

Ouch, it mixed diff kinds of problems. try again

Status: Needs review » Needs work

The last submitted patch, 69: 1988968-68.patch, failed testing.

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.

Status: Needs review » Needs work

The last submitted patch, 71: 1988968-71.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
14.06 KB

Status: Needs review » Needs work

The last submitted patch, 73: 1988968-72.patch, failed testing.

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.

Status: Needs review » Needs work

The last submitted patch, 78: 1988968-78-es6.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
17.3 KB

Status: Needs review » Needs work

The last submitted patch, 80: 1988968-80-es6.patch, failed testing.

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