Question

I have a small JS function that does Ajax for me and another like it that adds in POST data to the request. With Ajax being such a big topic with so many libraries about it, what am I missing from my function, is it insecure or something else worrying?

function loadPage(pagePath, displayElement)
{
    var xmlHttp;

    try
    {
        // Firefox, Opera 8.0+, Safari
        xmlHttp=new XMLHttpRequest();
    }
    catch (e)
    {
        // Internet Explorer
        try
        {
            xmlHttp=new ActiveXObject("Msxml2.XMLHTTP");
        }
        catch (e)
        {
            try
            {
                xmlHttp=new ActiveXObject("Microsoft.XMLHTTP");
            }
            catch (e)
            {
                alert("Your browser does not support AJAX!");
                return false;
            }
        }
    }

    xmlHttp.onreadystatechange=function()
    {
        if(xmlHttp.readyState==4)
        {
            document.getElementById(displayElement).innerHTML = xmlHttp.responseText;
        }
    }

    xmlHttp.open("GET", pagePath, true);
    xmlHttp.send(null);
}
Was it helpful?

Solution

I strongly recommend you not roll your own Ajax code. Instead, use a framework such as Prototype, Dojo, or any of the others. They've taken care of handling all the ReadyStates you're not handling (2 means it's been sent, 3 means it's in process, etc.), and they should escape the reponse you're getting so you don't insert potentially insecure javascript or something into your page.

Another thing a more robust framework will give you is the ability to do more than just use innerHTML to replace items in the DOM. Your function here can only be used to replace one element with the response from the ajax call. There's a lot more you can do with Ajax.

OTHER TIPS

I would remove this line.

alert("Your browser does not support AJAX!")

Shouting at the user in a language he probably doesn't understand is worse than failure. :-)

I've never been a fan of nested try/catch blocks, so I'd do it something like:

var xmlHttp;
if (window.XMLHttpRequest) {
  // Firefox, Opera 8.0+, Safari
  xmlHttp=new XMLHttpRequest();
} else if (window.ActiveXObject) {
  try {
    xmlHttp=new ActiveXObject("Msxml2.XMLHTTP");
  } catch (e) {
    xmlHttp=new ActiveXObject("Microsoft.XMLHTTP");
  }
}

if (xmlHttp) {
  // No errors, do whatever you need.
}

I think that'll work. But as has been mentioned before - why reinvent the wheel, use a library. Even better - find out how they do it.

jQuery is probably one of the lightest popular libraries out there.

The same thing in prototype:

function loadPage(pagePath, displayElement) {
    new Ajax.Updater(displayElement, pagePath);
}

Ajax.Updater in Prototype API

If you really want to see what you are missing, read the jQuery or Prototype source code for their ajax routines. If there are bug numbers in the comments, look those up as well.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top