19

I have a HTML5 application which is using jquery 3.2.1.

In part of the application - a search feature - I make an ajax request. The response from the ajax request is HTML, and this includes a <script> tag which links to a js file which is hosted on the same server as the application.

So the ajax code looks like this - for making the ajax request and writing the response to a div with the ID #ajaxContent:

$.ajax({
    url: $('#searchRegulations').attr('action'),
    type: 'post',
    cache: false,
    data: $('#searchRegulations').serialize()
 }).done(function (response, status, xhr) {
        if (response) {
            $('main .content').hide();
            $('#ajaxContent').html(response).show();
            return false;
        }
    }
});

If I inspect #ajaxContent I can see that the <script> tag is included in the ajax response:

enter image description here

I have also checked my Network tab to make sure /js/search_regulations.js is being loaded correctly, and it's giving a 200 response:

Inside search_regulations.js there is some jquery which toggles some filters that are present in #ajaxContent.

The problem is that this code only seems to be working about 50% of the time. When it works it will toggle the state of some filter buttons by adding/removing a class .active to elements inside .browse-ctp__filters-data and then writing them to a hidden form with the ID #tmpFilters.

To ensure the script was "firing" I put in the line console.log('search_regulations.js firing'); and sure enough this is shown in the Console every time irrespective of whether the script functions or not.

What's stranger is that if I cut/paste the code into my Console after the ajax response has been written to the page, it always works as expected.

Is this some issue with the way the script is being brought into the page?

I've pasted the script below, but I don't think it's an issue with the code in it, rather the way the browser/ajax response is being handled:

$(function() {  

console.log('search_regulations.js firing');

/* toggle the active (applied) state on browse by filters */
/* @link https://stackoverflow.com/questions/48662677/switch-active-class-between-groups-of-include-exclude-buttons */
$(document).on('click', '.browse-ctp__filters-data .include, .exclude', function(){

    var $this = $(this);

    // Split name into array (e.g. "find_355" == ["find", "355"]) 
    var arr = $this.attr('name').split('_');


    // Toggle active class
    $this.toggleClass("active");
    if ($this.siblings().hasClass("active")) {
      $this.siblings().removeClass("active")
    }

    // Remove any existing instances of the filter from hidden form
    $('#tmpFilters input[value="exclude_'+arr[1]+'"]').remove();
    $('#tmpFilters input[value="find_'+arr[1]+'"]').remove();

    // If a filter has been applied then add it to hidden form
    if ($this.hasClass('active')) {
        $('#tmpFilters').append('<input type="hidden" name="tmpFilter[]" value="'+$this.attr('name')+'">');
    }
});    
}); 

Notes about the Bounty offered:

I've offered a bounty because it's not a trivial problem to solve - demonstrated by the fact nobody has given a workable answer. I expect the correct answer to:

  1. Be demonstrable with jsfiddle or equivalent.
  2. Explain how/why it works.
  3. Understand that the ajax response is HTML and js. The js acts on HTML elements in the response. Therefore both the HTML and js need to be included in the response - as opposed to saying "just add the js to a global file" (I don't want the js to be global, because it's specific to the HTML response given, and can vary in different parts of the application).
  4. Should not use a timeout (setTimeout or whatever). If the user interacts with the UI elements - such as buttons - returned in the HTML response before the timeout and therefore js is fired... that just leads to the same problem we have now. So that isn't a valid solution as far as I'm concerned.
  5. If this problem is impossible to solve in HTML5/jquery explain why and suggest any alternative ways to handle it.

jsfiddle showing the HTML and script tag returned via ajax:

Several people have asked for a fiddle or demo. No 2 people would get the same outcome - which is very much the point of the question - so I didn't make one originally. The HTML returned that gets written to #ajaxContent is shown here: http://jsfiddle.net/v4t9j32g/1/ - this is what dev tools in the browser shows following the ajax response. Please note that the length of the content returned can vary, since it's the response to a keyword search facility that brings back a load of filter buttons. Also note that this HTML response contains the line <script src="/js/search_regulations.js"></script>. That is where the problematic js is located and the full contents of that are shown above in this question - the bit that includes console.log('search_regulations.js firing')

Andy
  • 5,142
  • 11
  • 58
  • 131
  • I don't think you need the `$(function() {` part, because this waits for the document to be ready. But if you make it to here, the document was ready a long time before. Try without that? – Jeremy Thille Jul 20 '18 at 12:13
  • 1
    Try throwing in 'defer' - ` – wazz Jul 20 '18 at 12:15
  • 3
    As wazz says this is most likely a race condition where your new elements haven't mounted into the DOM yet. Log the result of your selectors in that script and you'll likely see they aren't found when it doesn't work. PS returning HTML and even worse a script in an ajax response is very old school and frowned upon, better to return raw JSON data and render it out how you like. – Dominic Jul 23 '18 at 09:06
  • 1
    @DominicTobias thanks. The reason I've offered a bounty is because I need a fully worked example which explains all of those concepts in detail. Particularly returning JSON and rendering it...how does that help overcome the issue with a ` – Andy Jul 23 '18 at 09:54
  • If you receive it as a JSON response then you would simply render the results out to your page in the current script. The reason people do that is that it makes your API more flexible and keeps concerns separated. You could use the same API on mobile, or two websites for instance. Did you try adding `defer` attribute to your script? – Dominic Jul 23 '18 at 10:16
  • Are those css selectors used in the dynamically loaded script in the same HTML template? – Rahul Raut Jul 23 '18 at 17:31
  • getScript function of jquery may help you. – Mohanlal Prajapati Jul 24 '18 at 03:34
  • 1
    Any way you can create a working example of this happening? @Andy – Naftali Jul 24 '18 at 19:54
  • @Neal not really - the whole point is that it doesn't happen *all* of the time. It happens at random. I think it's as Dominic Tobias was saying where sometimes the elements haven't mounted to the DOM. – Andy Jul 25 '18 at 08:39
  • 2
    You can prove if it is a race condition or not, by wrapping the changes that search_regulations.js makes in a SetTimeout method. Set it to delay like 5000ms. If this makes things work perfectly every time. Then you know for sure that it is a race condition. – Blair Jul 25 '18 at 14:09
  • @Blair it seems very likely it is a race condition. But the question still remains - how do you handle this situation (without using a timeout etc)? Basically I want to see a solution which brings in content from an ajax response, writes it to a page, and then have the js act on it. But the js needs to be included within the ajax response because I don't want/need it there "globally" for the entire application. The js in this case is specific to something returned in the response. So... how to handle that? – Andy Jul 25 '18 at 14:29
  • Maybe wrap it in setTimeout and play with the intervals, until you have the sweet spot. – Amiga500 Jul 25 '18 at 14:56
  • I've added some comments about the bounty. Using a timeout isn't a valid solution - explained in the edited question. – Andy Jul 25 '18 at 15:04
  • please add one more log below the exisint one and report back: console.log($('.browse-ctp__filters-data .include, .exclude').length), i am almost certain that for some reason, the elements you are attaching on are not present in the dom – MKougiouris Jul 25 '18 at 15:07
  • 1
    The fact that 2 people won't get the same results is no excuse for not providing an [MCVE]. First, by doing this, **you** might find what causes the issue. Second, some of us don't need to actually run it. But reading it exactly and completely as it is written is mandatory. – Kaiido Jul 25 '18 at 15:08
  • @Andy I've answered quite a few race condition questions in my time. It is sensible to start with the information you've shown so far but there's no glaring issue there. So there must be something else contributing to the problem. Hence the calls for a live example. It is true that race conditions are hard to reproduce, but if you can put something together which *at your end* reproduces the problem, even intermittently, that's already something. It is possible for someone else, even if they cannot themselves reproduce the issue, to mentally walk your code and find the problem. – Louis Jul 25 '18 at 15:08
  • @Andy I'm not sure what the constraints are, but I wanted to make sure you understand that bundle a html + js as you said "HTML and js need to be included in the response" and "can vary in different parts of the application" that just wrong architecture of dynamic web apps. – Or Duan Jul 25 '18 at 15:08
  • @Or Duan - I accept that you may have a point. But instead of people just saying "you've done this the wrong way" please post a full answer that demonstrates how to actually do it. Lots of people are telling me I'm wrong - yes I know, that's why I've asked the question in the first place! – Andy Jul 25 '18 at 15:11
  • @Andy All good man, we're all friends here! Just wanted to make sure we're on the same page and you're willing to hear a different approach. And no, I don't have an answer for your direct issue, I'm willing to try to help with an alternative. – Or Duan Jul 25 '18 at 15:15
  • @Or Duan thank you, I'm willing to hear any approach that helps solve the problem at this stage. I accept the way I'm going about it may be totally wrong. So if there's an alternative/better way of doing this type of thing then yes I'd like to know. – Andy Jul 25 '18 at 15:16
  • set ajax´s `dataType` property to `html` and the scritp should execute, if not you have to put the actual script inside the script tags, no src attribute and it will 100% work – john Smith Jul 25 '18 at 15:22
  • Maybe you can extract the script url from the html response and use the jquery function 'loadScript; like => $.loadScript('url_to_someScript.js', function(){ //Stuff to do after someScript has loaded }); – Bert Maurau Jul 25 '18 at 15:26
  • 1
    I don't understand why people are talking about race conditions when the script isn't attaching events directly to the new DOM nodes. It's using delegation and only attaching an event to the document. jQuery checks on document click if the event target matches the `.browse-ctp__filters-data .include, .exclude` query. It doesn't matter if those elements aren't in the DOM at the time of adding the click handler. – yts Jul 26 '18 at 01:34
  • @Andy can we see the html in question? How many times are you making this ajax call while on the page? – yts Jul 26 '18 at 01:43
  • I've added a fiddle to show an example of the HTML returned via ajax - http://jsfiddle.net/v4t9j32g/1/. This gets written to `#ajaxContent`, so is shown contained within it - this is basically what the dev tools in the browser show inside that div after the ajax request has competed. Hope that helps. – Andy Jul 26 '18 at 14:02
  • 1
    @Andy what about bastos.sergio's answer? Are you making this ajax call more than once? – yts Jul 26 '18 at 16:17
  • @yts please do realize that even if "the script isn't attaching events directly to the new DOM nodes" the script in the response is continuously adding new listeners to the same document node every time a new Ajax request is performed. I believe Sally CJ showed the issue without leaving place to doubts. – Diego Perini Jul 29 '18 at 20:13
  • @DiegoPerini I was referring to the commentators who were suggesting that the problem was the script was running too early, before the new html was in the DOM. See the previous comments about setTimeouts and race conditions. – yts Jul 30 '18 at 13:46
  • @yts, you are correct there. That's surely not the problem since the listener is attached later, after the first Ajax response is retrieved and the script executed. – Diego Perini Jul 31 '18 at 01:20

8 Answers8

6

One of the problems that I see is that you're binding the onclick to the same elements multiple times...

Since the js can be loaded multiply times via ajax requests, it is important to first detach, before attaching again events.

The other problem, is that you're running the code in $(document).ready event (that is when HTML-Document is loaded and DOM is ready), however you'd probably be better off to run the code in the $(window).load event (which executes a bit latter, when complete page is fully loaded, including all frames, objects and images)

Example:

$(window).load(function() {  

    console.log('search_regulations.js firing');

    //off: detach the events
    //on: attach the events
    $('.browse-ctp__filters-data .include, .exclude').off('click').on('click', function(){
        ...
    }
}); 
bastos.sergio
  • 6,684
  • 4
  • 26
  • 36
  • 2
    I agree with your first point. If the code is loaded an even number of times, the `toggle` will reverse itself and it would look like nothing happened. The `.load` part shouldn't matter because all this is being done after an ajax call, and it doesn't matter if the elements exist (the event handler is being attached to the document, not the dom elements). One modification though: instead of calling `.off('click')` (which might remove other click handlers), the event should be given a namespace so it's save to remove. `).off('click.toggleActive').on('click.toggleActive', ...)` – yts Jul 26 '18 at 01:50
2

In addition to what everybody were talking in the comments, I'll try to put it into an answer.

Event driven

JS is event driven, that doesn't mean you have to load scripts on the fly to make them execute functions when an event is fired. A better approach would be to load everything you need on the page load, and add event listeners, this will case much better user experience(less http requests) and much better code matinability.

TL;DR

In general I would do something like that in you html file:

<script src="/js/main.js">  
<script src="/js/search_regulations.js" async>  <!-- bonus tip! google "async script tag" -->

This will load all the js you need.

Keep in mind that search_regulations.js should add the event listener you want with jQuery on method, but since the html didn't exist when the script added the event listener, you might want to check this.

Why this is better?

  • Now you had pretty easy case where one file loads another. In the future you might want to add more features and more complex code. This will be painful to debug when you have chains of scripts that load each other.
  • The user has to wait until the request for the search_regulations.js file to be loaded, if they're on the a mobile network/the file will get bigger in size that might be a poor user experience.
  • You can still reuse your code over the application with search_regulations.js
Or Duan
  • 13,142
  • 6
  • 60
  • 65
2

NOTE:

  • @bastos.sergios already answered your question — see his first point.
  • Credit should also be given to @yts for "strengthening" Bastos's answer.

So the following should fix the problem with your search_regulations.js script:

$(document)
  // Detach the event handler to prevent multiple/repeat triggers on the target elements.
  .off('click.toggleActive', '.browse-ctp__filters-data .include, .exclude')
  // Attach/re-attach the handler.
  .on('click.toggleActive', '.browse-ctp__filters-data .include, .exclude', function(){
    ... put your code here ...
  });

But my primary intention of writing this answer, is to let you see the problem with your script.

And if you're curious, here's script I used with that demo, which slightly modified:

https://codepen.io/anon/pen/RBjPjw.js

Sally CJ
  • 15,362
  • 2
  • 16
  • 34
2

About the requirements

The solution to the problem is available using either HTML5/jQuery or a combination of a previous HTML version and native Javascript. The only reason to use a library like jQuery is to bring older browser to the same level of support or to fix bugs and to have a standard framework shared between groups of developers.

Also, in my opinion, it doesn't matter where you place the few lines of code needed to solve the problem, they can be put in the head section of the page, at the bottom of the page or as for the requirements of the bounty inside the Ajax response payload that is reloaded frequently.

About the issue

The real trick lies in the method you use to achieve the firing of click events, your choice of doing that inside the script present in the repeated Ajax responses is what makes your cade fail at some point.

Each time an Ajax request happens the script in the response adds new event listeners. These will continuously sum up until the browsers fails for some reason (not enough memory, exhausted event stack, or click events race conditions).

About the solution

So, the solution to your issue would be to avoid adding events each time the Ajax request/response happens. This is a task which can be easily solved by a simple "capturing phase event delegation" (not frequently used unfortunately).

An event listener is added only once on one of the top nodes which can be the "document" node itself or the "html" element or the "body" element, thus on one element that is always present in the DOM after the initial page load.

This element will be in charge of capturing all the click on the page, currently present or loaded at a later time (by your Ajax requests), and this element will act that way for the complete life span of the page, no need to setup/destroy events as I see some suggested. Well that will probably work as well but will generate more workload on the browser engine, consume more memory, require more code and overall be slower and unnecessarily tricky to say the least.

Code samples

I have setup two CodePen example for you:

Delegation1 has event handling code in the HEAD section, it is my preferred way to handle these kind of issues if no restrictions or other unknown quirks exists. The benefits with this version is shorter/faster and more maintainable code.

Delegation1

Delegation2 has event handling code in the Ajax response and should meet the requirements about HTML/JS being loaded continuously. There is a check to only install the listener once and avoid multiple events racing & overlapping.

Delegation2

The Javascript code in the examples contains relevant comments and should be enough to explain the implemented strategy to make this as simple as possible and reusable in other similar cases.

These examples were written with older browsers in mind, newer browser expose more powerful APIs like element.matches(selector) very useful with event delegation. I purposely avoided to use it for broader support.

Diego Perini
  • 8,003
  • 1
  • 18
  • 9
0

You need to strip the < script > tag out of the HTML and create a script tag manually to add to the DOM. Essentially, you can convert the Html to JS via:

var $html = $(data.Html)

Then, pull out all the script tags like this:

 var scripts = [];
        $html.each(function(i, item) {
            if (item instanceof HTMLScriptElement) {
                scripts.push(item);
            }
        });

Then you need to remove all the < script > tags from the html before appending it to the DOM.

$html.find("script").remove();

$("placeToAddHtml").append($html);

Once the HTML, stripped of the tags is added, you can then manually add each script to the DOM at the end:

           for (var i = 0; i < scripts.length; i++) {
                var script = document.createElement('script');

                $(scripts[i]).each(function() {
                    $.each(this.attributes, function(j, attrib) {
                        var name = attrib.name;
                        var value = attrib.value;
                        script[name] = value;
                    });
                });

                script.text = scripts[i].innerHTML;
                document.body.appendChild(script);
            }

Hope this works how you intend it!

Edit: You may need to build up the script tag here in a way that appends all of the pulled out scripts at the same time if they depend on each other. That way, you only add one giant script tag that has all the other tags internally at once.

Daniel Lorenz
  • 4,178
  • 1
  • 32
  • 39
0

To avoid the possibility of a race condition without implementing a flat timer, you will need to return an ajax response that separates the script from the html, such as a json object that returns two elements (the script or an array of scripts to support cases where you need multiple, and a second element that contains the stringified html markup).

Sample json response from ajax:

{
    "script": "/yourscript.js",
    "html": "<p>Your markup...</p>"
}

This (parsed) json will be referred to as xhrResponse below for brevity.

Before applying either to the dom, append a preload tag for your script(s) prior to loading the dom element, so they will start being resolved in the background without loading, which should minimize the overall loading timeline, and help alleviate much of the possibility of ongoing race conditions:

document.getElementsByTagName('head')[0].appendChild( '<link rel="preload" href="' + xhrResponse.script + '" as="script">' );

This will begin loading your script in the background without executing it, so it can be applied immediately when you apply the script tag to the dom later.

You will then want to append the dom elements next, and then apply the scripts after the new dom content has resolved. You may alternately pass the markup directly in the json response, or alternately return a link to a template file and serve that in conjunction with the above mentioned preload method for the template itself. If you are more concerned with the possibility of escaping issues on content, do the latter. If you are more concerned with bandwidth and latency, do the former. There are some minor caveats to both, depending on your use case.

document.getElementsByTagName('head')[0].appendChild( xhrResponse.html );

You will then want to check that the applied html content has resolved within the dom. An arbitrary wait timer is not a very good way to do this, as it has a high likelihood of making the user wait longer than is necessary, and occasional network bottlenecks may also cause your script to occasionally choke if the content does not resolve prior to your arbitrary timer length (more of an issue if you are serving a template from a link instead of stringified html markup), or if the end user has low memory currently available (old machines, bzillion tabs open, or very large html payloads even when served directly stringified).

Please see this answer for a pretty robust solution to check if dynamically added dom content has resolved correctly.

Once this has resolved, then apply the script elements afterward with something like this:

var script = document.createElement('script');
script.src = xhrResponse.script;
script.async = true; // use false here if you want synchronous loading for some reason
document.head.appendChild(script);

The earlier mentioned preload tag should insure that your script has already been localized by the browser by this point, or is very nearly done doing so already. You can then check if it has completed with a ready state event listener if it is still giving you issues:

script.onreadystatechange = function() {
    if (script.readyState === "complete") {
        // debug stuff, or call an init method for your js if you really want to be sure it fired after everything else is done.
    }
}

It should be noted that in the above ready state listener, if the preload resolves prior to this portion of your logic, the ready state will not change (because it is already done), and this portion of logic will not execute. Depending on the weight of your DOM content, this may or may not apply to you.


This approach will insure loading in the correct order, and also decouple your script and markup, so either can be recycled independently of each other elsewhere if applicable now or at any point in the future either.

mopsyd
  • 1,877
  • 3
  • 20
  • 30
-1

Thinked that if you add this to your ajax params call all will be ok:

async: false

Felipe Mora
  • 42
  • 1
  • 11
-1

The problems i think could be: 1. Include and exclude triggers the same function. Error in logic inside to handle both. (since you told it works 50% of the time). 2.Try updating the DOM after you have appended the HTML.

   componentHandler.upgradeDom();

just after

 $('#ajaxContent').html(response).show();

Hope it helps !!!

Codex
  • 155
  • 6