views:

162

answers:

6

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);
}
+11  A: 

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.

MattGrommes
+5  A: 

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. :-)

Patrick McElhaney
A: 

@MattGrommes
Which of the JS libraries is the lightest? I don't want to use anything too heavy.

@Patrik McElhaney
You make a valid point ;)

Teifion
+1  A: 

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

ceejayoz
+3  A: 

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.

Lee Theobald
+1  A: 

The same thing in prototype:

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

Ajax.Updater in Prototype API

erlando