JavaScript ajax library critique

[Mychal Hackman]

I am a new JavaScript programmer. In training I was introduced to
Prototype and in the first ~8 months of my job I heavily relied on it.
Over time I realized how ridiculous it was to include over 4000 lines
of code for event handling and XHR. (As well as I didn't think script
errors on every page seemed professional.)

I've started writing my own library, and I originally attempt to make
it appear similar to Prototype, in the hopes that I could easily get
the other developers off Prototype and eventually improve our site.

The following is the "Ajax" section of the code, and I would greatly
appreciate (and request) critiques of the code from the group.

Ajax = {
transport : (function(){
if(window.XMLHttpRequest){
return new XMLHttpRequest();
}else if(window.ActiveXObject){
return new ActiveXObject('Microsoft.XMLHTTP');
}else{
return false;
}
})(),

encodeURIParams : function(params){
var url='',
noCACHE = Math.random()*10e17,
key, i;

for(key in params){
if(params.hasOwnProperty(key) && key != 'noCACHE'){
//in the case of a select multiple
if(typeof params[key] == 'object' &&
params[key] instanceof Array){
for(i = params[key].length; i--{
url += encodeURIComponent(key) + '=' +
encodeURIComponent(params[key])+'&';
}

}else{
url += encodeURIComponent(key) + '=' +
encodeURIComponent(params[key])+'&';
}
}
}

if(!params.hasOwnProperty('noCACHE')){
url += 'noCACHE='+noCACHE;
}else{
url = url.substring(0, url.length-1);
}

return url;
},

Request : function(url, params, onsuccess, method){
var xhr = Ajax.transport,
res = '',
post = null;

xhr.onreadystatechange = function(){
if(onsuccess && xhr.readyState==4){
res = xhr.responseText ||
xhr.responseJSON ||
xhr.responseXML;

onsuccess(res);
}
};

if(method && method.toUpperCase() == 'POST'){
params.noCACHE = false;
post = Ajax.encodeURIParams(params);
xhr.open(method, url, true);
xhr.setRequestHeader('Content-type', 'application/x-www-
form-urlencoded');
xhr.setRequestHeader('Content-length', post.length);
xhr.setRequestHeader('Connection', 'close');
}else{
method = 'GET';
xhr.open(method, url+'?'+Ajax.encodeURIParams(params),
true);
}
xhr.send(post);
}
}

 

[David Mark]

I am a new JavaScript programmer. In training I was introduced to
Prototype and in the first ~8 months of my job I heavily relied on it.
Over time I realized how ridiculous it was to include over 4000 lines
of code for event handling and XHR. (As well as I didn't think script
errors on every page seemed professional.)

No question. But these days it is typically jQuery fouling up behind
the scenes, not Prototype.

I've started writing my own library, and I originally attempt to make
it appear similar to Prototype, in the hopes that I could easily get
the other developers off Prototype and eventually improve our site.

I sure wouldn't copy Prototype's much-maligned design.

The following is the "Ajax" section of the code, and I would greatly
appreciate (and request) critiques of the code from the group.

Ajax = {
    transport : (function(){
        if(window.XMLHttpRequest){

Bad feature detection. Use typeof (or a wrapper like isHostMethod).

            return new XMLHttpRequest();
        }else if(window.ActiveXObject){
            return new ActiveXObject('Microsoft.XMLHTTP');

You need a try-catch around this (ActiveX is easily disabled) and
there are more ID's to consider.

        }else{
            return false;
        }
    })(),

    encodeURIParams : function(params){
        var url='',
            noCACHE = Math.random()*10e17,

Why? There are headers for this.

            key, i;

        for(key in params){
            if(params.hasOwnProperty(key) && key != 'noCACHE'){

Don't use hasOwnProperty as it isn't supported by Safari 2 (for
example). Use a wrapper like isOwnProperty.

                //in the case of a select multiple
                if(typeof params[key] == 'object' &&
                    params[key] instanceof Array){

Don't use instanceof for this (and don't design code that must
discriminate between Array objects and Object objects). What other
sort of object could this be anyway?

                    for(i = params[key].length; i--{
                        url += encodeURIComponent(key) + '=' +
                            encodeURIComponent(params[key])+'&';
                    }

                }else{
                    url += encodeURIComponent(key) + '=' +
                        encodeURIComponent(params[key])+'&';
                }
            }
        }

Faster to push the names, values and separators to an array and then
join with "".

        if(!params.hasOwnProperty('noCACHE')){

See above. And if it does _not_ have this property, wouldn't that
mean to cache? It's very confusing to have this magic parameter in
with the rest of the data. Also, the caching behavior for XHR GET's
varies cross-browser (IE doesn't bother to check freshness).

            url += 'noCACHE='+noCACHE;

Never do this. Certainly not for data that will be POSTed.

        }else{
            url = url.substring(0, url.length-1);
        }

        return url;
    },

    Request : function(url, params, onsuccess, method){

This is not a constructor.

        var xhr = Ajax.transport,

Do you really need a "namespace" for this?

            res = '',
            post = null;

        xhr.onreadystatechange = function(){
            if(onsuccess && xhr.readyState==4){
                res = xhr.responseText ||
                      xhr.responseJSON ||
                      xhr.responseXML;

This is bizarre.

                onsuccess(res);
            }
        };

        if(method && method.toUpperCase() == 'POST'){
            params.noCACHE = false;

What does this mean, caching is true for POST's? I doubt it.

            post = Ajax.encodeURIParams(params);

But you put the cache-buster BS in there!

            xhr.open(method, url, true);
            xhr.setRequestHeader('Content-type', 'application/x-www-
form-urlencoded');
            xhr.setRequestHeader('Content-length', post.length);
            xhr.setRequestHeader('Connection', 'close');

Why add these headers?

        }else{
            method = 'GET';
            xhr.open(method, url+'?'+Ajax.encodeURIParams(params),
true);

What if the URI already had a query part?

        }
        xhr.send(post);

Need a try-catch around that.

    }

}

Will leak memory in IE too. You need to set onreadystatechange to an
empty function when done.

The people you are trying to get off Prototype will complain that you
are "reinventing the wheel". For them there are only two choices: use
a ridiculously complex and ineffectual library or write _everything_
from scratch. You need to find some middle ground (e.g. download a
solid XHR wrapper to start with).

 

[Garrett Smith]

I am a new JavaScript programmer. In training I was introduced to
Prototype and in the first ~8 months of my job I heavily relied on it.
Over time I realized how ridiculous it was to include over 4000 lines
of code for event handling and XHR. (As well as I didn't think script
errors on every page seemed professional.)

I've started writing my own library, and I originally attempt to make
it appear similar to Prototype, in the hopes that I could easily get
the other developers off Prototype and eventually improve our site.

The following is the "Ajax" section of the code, and I would greatly
appreciate (and request) critiques of the code from the group.

Ajax = {
transport : (function(){
if(window.XMLHttpRequest){
return new XMLHttpRequest();
}else if(window.ActiveXObject){
return new ActiveXObject('Microsoft.XMLHTTP');
}else{
return false;
}
})(),

Be careful to avoid potential circular reference of Host object to
JScript object. You have Ajax.transport -> Host object...

encodeURIParams : function(params){
var url='',
noCACHE = Math.random()*10e17,
key, i;

for(key in params){

What is - params - ? Is it a native ECMAScript object? If so, and if
you're not modifying object.prototype, you can ditch the call to
hasOwnProperty.

if(params.hasOwnProperty(key) && key != 'noCACHE'){
//in the case of a select multiple
if(typeof params[key] == 'object' &&
params[key] instanceof Array){
for(i = params[key].length; i--{

// Move the repeated encodeURIComponent(key) outside the loop and
// save the result in a variable (like encodedKey).

url += encodeURIComponent(key) + '=' +
encodeURIComponent(params[key])+'&';
}

}else{
url += encodeURIComponent(key) + '=' +
encodeURIComponent(params[key])+'&';
}
}
}

Avoid the if else. Make

if(!params.hasOwnProperty('noCACHE')){
url += 'noCACHE='+noCACHE;
}else{
url = url.substring(0, url.length-1);
}

return url;
},

Request : function(url, params, onsuccess, method){
var xhr = Ajax.transport,
res = '',
post = null;

xhr.onreadystatechange = function(){

I believe you have created a circular reference here.

xhr has an onreadystatechange, which has xhr.

setting xhr = null will not solve the problem here. onreadystatechange
has Ajax.transport in scope chain.

Ajax.transport -> onreadystatechange -> [[Scope]] -> Ajax.transport.

if(onsuccess && xhr.readyState==4){
res = xhr.responseText ||
xhr.responseJSON ||
xhr.responseXML;

onsuccess(res);
}
};

Why not just use responseText?

What if responseText is ""?

What is responseJSON?

if(method && method.toUpperCase() == 'POST'){
params.noCACHE = false;

Caching can be useful at times. Consider an application where the user
enters home zip, looks at result, then tries work zip code, looks at
result, then re-enters home zip. The home zip being cached would be useful.

 

[Thomas 'PointedEars' Lahn]

Be careful to avoid potential circular reference of Host object to
JScript object. You have Ajax.transport -> Host object...

Where is the circular reference? And how would you avoid it, then?


PointedEars

 

[David Mark]

Be careful to avoid potential circular reference of Host object to
JScript object. You have Ajax.transport -> Host object...

It's the start of a chain.

    encodeURIParams : function(params){
        var url='',
            noCACHE = Math.random()*10e17,
            key, i;

        for(key in params){

What is - params - ? Is it a native ECMAScript object? If so, and if
you're not modifying object.prototype, you can ditch the call to
hasOwnProperty.

            if(params.hasOwnProperty(key) && key != 'noCACHE'){
                //in the case of a select multiple
                if(typeof params[key] == 'object' &&
                    params[key] instanceof Array){
                    for(i = params[key].length; i--{

// Move the repeated encodeURIComponent(key) outside the loop and
// save the result in a variable (like encodedKey).

                        url += encodeURIComponent(key) + '=' +
                            encodeURIComponent(params[key])+'&';
                    }

                }else{
                    url += encodeURIComponent(key) + '=' +
                        encodeURIComponent(params[key])+'&';
                }
            }
        }

Avoid the if else. Make

        if(!params.hasOwnProperty('noCACHE')){
            url += 'noCACHE='+noCACHE;
        }else{
            url = url.substring(0, url.length-1);
        }

        return url;
    },

    Request : function(url, params, onsuccess, method){
        var xhr = Ajax.transport,
            res = '',
            post = null;

        xhr.onreadystatechange = function(){

I believe you have created a circular reference here.

Yes.

[xmlhttp]--readystatechange-->[anon function]-->[Activation Object]-->
[xmlhttp]

But this is unrelated to the previously mentioned (partial) chain.

xhr has an onreadystatechange, which has xhr.

setting xhr = null will not solve the problem here. onreadystatechange
has Ajax.transport in scope chain.

That won't solve the problem because it will blow up in IE6. You have
to set it to a Function object (preferably an empty one).

Ajax.transport -> onreadystatechange -> [[Scope]] -> Ajax.transport.

No. Ajax is global. It's not referenced by the local Activation
Object. If it were, virtually everything would leak (think about it).

Why not just use responseText?

You might want the XML.

onsuccess(xhr.resonseText, xhr.responseXML);

Or just:-

onsuccess(xhr);

What if responseText is ""?

Nothing good.

What is responseJSON?

Who knows? Something Prototype does?

Caching can be useful at times. Consider an application where the user
enters home zip, looks at result, then tries work zip code, looks at
result, then re-enters home zip. The home zip being cached would be useful.

Caching POST's? I don't follow. And caching GET's is a major pain
because of IE's problems. So caching XHR is best avoided.

 

[David Mark]

[...]



Will leak memory in IE too.  You need to set onreadystatechange to an
empty function when done.

(Such as Function.prototype)

That's an interesting idea. I've always used an empty function.

 

[Mychal Hackman]

No question.  But these days it is typically jQuery fouling up behind
the scenes, not Prototype.

Yeah, that has been recommended to me quite often lately.

Bad feature detection.  Use typeof (or a wrapper like isHostMethod).


You need a try-catch around this (ActiveX is easily disabled) and
there are more ID's to consider.

I recall having trouble finding information about the ID's, and what
the differences were.

Why?  There are headers for this.

I have removed the noCACHE parts, caching will be prevented if
necessary by setting the headers server-side.

Don't use hasOwnProperty as it isn't supported by Safari 2 (for
example).  Use a wrapper like isOwnProperty.

Thanks, I'll look into that.

                //in the case of a select multiple
                if(typeof params[key] == 'object' &&
                    params[key] instanceof Array){

Don't use instanceof for this (and don't design code that must
discriminate between Array objects and Object objects).  What other
sort of object could this be anyway?
Good question...

Faster to push the names, values and separators to an array and then
join with "".

Makes sense.

This is not a constructor.

Sorry, I don't follow here. Can you elaborate please?

This is bizarre.

Agreed. Not sure where responseJSON even came from...

Why add these headers?

Removed last two. Obviously wasn't thinking (just following blindly).

Will leak memory in IE too.  You need to set onreadystatechange to an
empty function when done.

Thanks again. I will have to read about circular references in IE and
try to understand this more clearly.

a ridiculously complex and ineffectual library or write _everything_
from scratch.

Writing some of this will (hopefully) give me a better understanding
of the language and improve my skills as a programmer.

 

[David Mark]

Where is the circular reference?  And how would you avoid it, then?

There is none and there would be no way to avoid it if there was.

 

[Thomas 'PointedEars' Lahn]

Mychal Hackman wrote:
[...]
Will leak memory in IE too. You need to set onreadystatechange to an
empty function when done.

(Such as Function.prototype)

That's an interesting idea. I've always used an empty function.

There is also a subtle difference between ES3F and ES5 that challenges
whether using `Function.prototype' here is still a viable approach:

,-[ECMAScript Language Specification, Edition 3 Final]
|
| 15.3.4 Properties of the Function Prototype Object
|
| The Function prototype object is itself a Function object (its [[Class]]
| is "Function") that, when invoked, accepts any arguments and returns
| undefined.
|
| The value of the internal [[Prototype]] property of the Function prototype
| object is the Object prototype object (section 15.3.2.1).
|
| It is a function with an “empty bodyâ€; if it is invoked, it merely returns
| undefined.
|
| The Function prototype object does not have a valueOf property of its own;
| however, it inherits the valueOf property from the Object prototype
| Object.

,-[ECMAScript Language Specification, Edition 5
| ("Final final final final draft"¹)]
|
| 15.3.4 Properties of the Function Prototype Object
|
| The Function prototype object is itself a Function object (its [[Class]]
| is "Function") that, when invoked, accepts any arguments and returns
| undefined.
|
| The value of the [[Prototype]] internal property of the Function prototype
| object is the standard built-in Object prototype object (15.2.4). The
| initial value of the [[Extensible]] internal property of the Function
| prototype object is true.
|
| The Function prototype object does not have a valueOf property of its own;
| however, it inherits the valueOf property from the Object prototype
| Object.
|
| The length property of the Function prototype object is 0.

Note that the part about the empty body is gone from ES5 which means that a
ES5-conforming implementation could actually do something when this method
is called as long as it accepts any arguments and returns `undefined'.


PointedEars
___________
¹ in the title of the corresponding PDF document although not advertised as
such

 

[Garrett Smith]

I am a new JavaScript programmer. In training I was introduced to
[...]

The following is the "Ajax" section of the code, and I would greatly
appreciate (and request) critiques of the code from the group.

Ajax = {

Don't make assignments to undeclared identifiers (don't forget var).

 

[Garrett Smith]

Mychal Hackman wrote:

[...]

I believe you have created a circular reference here.

Yes.

[xmlhttp]--readystatechange-->[anon function]-->[Activation Object]-->
[xmlhttp]

But this is unrelated to the previously mentioned (partial) chain.

There are two references to the XMLHttpRequest object: xhr and
Ajax.transport. Both are in scope of the function that is assigned to
onreadystatechange.

That won't solve the problem because it will blow up in IE6. You have
to set it to a Function object (preferably an empty one).

Setting xhr = null won't blow up in IE. Setting xhr.onreadystatechange =
null will, but setting xhr = null won't.

Setting xhr won't break the circle, either. There is still
Ajax.transport in scope of the function assigned to xhr.onreadystatechange.

Ajax.transport -> onreadystatechange -> [[Scope]] -> Ajax.transport.

No. Ajax is global. It's not referenced by the local Activation
Object. If it were, virtually everything would leak (think about it).

Identifier Ajax does not need to be referenced by the local activation
object. It is in the scope chain.

For example, if the onreadystatechange has an eval:-

xhr.onreadystatechange = function(){
if(Math.random > .999) {
eval("alert(typeof Ajax.transport)");
}
}

- it would still have to look up the scope chain and find
Ajax.transport. It makes no difference if the Ajax object is global. The
Ajax identifier is in the scope chain of the function assigned to
onreadystatechange.

The onreadystatechange has Ajax.transport and Ajax.transport has
onreadystatechange.

You might want the XML.

onsuccess(xhr.resonseText, xhr.responseXML);

Or just:-

onsuccess(xhr);

Passing responseXML when responseText is empty string would be an
unlikely problem case for the caller to handle. Best avoid that.

[...]

It is an odd design to have a globally accessible Ajax.transport object,
and then pass that very same object to onsuccess. I think the global
object should be gotten rid of.


IE6 (and 7?) throws errors when calling send on an XHR that has been
opened. TO reuse the object, it is necessary to fist call the xhr's
abort method.

Caching POST's? I don't follow. And caching GET's is a major pain
because of IE's problems. So caching XHR is best avoided.

I misread. Caching POST would be odd.

 

[Garrett Smith]

Where is the circular reference? And how would you avoid it, then?

It occurs below, as I believed it would. I wrote "potential". I did not
write that the circle was already established.

I try to creating references from host object to jscript object, in
general.

To avoid a leak, either don't create one or break the chain between host
object to jscript object.

 

[Garrett Smith]

There is none and there would be no way to avoid it if there was.

No, actually a circular reference does exist below. One of the
identifiers in scope of that reference is Ajax.transport.

 

[Gregor Kofler]

Mychal Hackman meinte:

Sorry, I don't follow here. Can you elaborate please?

By convention, upper case function names are reserved for constructors.

Gregor

 

[David Mark]

Mychal Hackman wrote:

[...]

    Request : function(url, params, onsuccess, method){
        var xhr = Ajax.transport,
            res = '',
            post = null;
        xhr.onreadystatechange = function(){
I believe you have created a circular reference here.

Yes.

[xmlhttp]--readystatechange-->[anon function]-->[Activation Object]-->
[xmlhttp]

But this is unrelated to the previously mentioned (partial) chain.

There are two references to the XMLHttpRequest object: xhr and
Ajax.transport. Both are in scope of the function that is assigned to
onreadystatechange.

That won't solve the problem because it will blow up in IE6.You have
to set it to a Function object (preferably an empty one).

Setting xhr = null won't blow up in IE. Setting xhr.onreadystatechange =
null will, but setting xhr = null won't.

That's correct. I misread.

Setting xhr won't break the circle, either. There is still
Ajax.transport in scope of the function assigned to xhr.onreadystatechange.

But setting onreadystatechange to an empty Function will. That's the
most important point.

Ajax.transport -> onreadystatechange -> [[Scope]] -> Ajax.transport.

No.  Ajax is global.  It's not referenced by the local Activation
Object.  If it were, virtually everything would leak (think about it)..

Identifier Ajax does not need to be referenced by the local activation
object. It is in the scope chain.

That's irrelevant. There's no circular reference in that case.

For example, if the onreadystatechange has an eval:-

xhr.onreadystatechange = function(){
   if(Math.random > .999) {
     eval("alert(typeof Ajax.transport)");
   }

}

- it would still have to look up the scope chain and find
Ajax.transport. It makes no difference if the Ajax object is global.

Of course it does. Diagram it.

The
Ajax identifier is in the scope chain of the function assigned to
onreadystatechange.

As mentioned, that is irrelevant to the memory leak consideration.

The onreadystatechange has Ajax.transport and Ajax.transport has
onreadystatechange.

You might want the XML.

onsuccess(xhr.resonseText, xhr.responseXML);

Or just:-

onsuccess(xhr);

Passing responseXML when responseText is empty string would be an
unlikely problem case for the caller to handle. Best avoid that.

[...]

It is an odd design to have a globally accessible Ajax.transport object,
and then pass that very same object to onsuccess. I think the global
object should be gotten rid of.
Certainly.


IE6 (and 7?) throws errors when calling send on an XHR that has been
opened. TO reuse the object, it is necessary to fist call the xhr's
abort method.

Caching POST's?  I don't follow.  And caching GET's is a major pain
because of IE's problems.  So caching XHR is best avoided.

I misread. Caching POST would be odd.

Yes.

 

[David Mark]

It occurs below, as I believed it would. I wrote "potential". I did not
write that the circle was already established.

But it does _not_ occur because of this potential (which is never
realized). That's a very important distinction.

I try to creating references from host object to jscript object, in
general.

To avoid? Yes, I do too. In light of IE's problems in this area,
it's the only sane approach. Just like you should avoid overloading
due to language constraints. Nevertheless, both of those bee-lines to
disaster are buzzing with activity (pick up any library or framework).

To avoid a leak, either don't create one or break the chain between host
object to jscript object.

Exactly.

 

[David Mark]

No, actually a circular reference does exist below. One of the
identifiers in scope of that reference is Ajax.transport.

As mentioned, that doesn't matter. Think about it (or draw a diagram
if in doubt).

 

[Mychal Hackman]

I have modified the code using information presented in this
discussion:

var Ajax = (function(){

var getXhr = (function(){
if(typeof window.XMLHttpRequest != 'undefined'){
return new XMLHttpRequest();

//lookup other progIds
}else if(typeof window.ActiveXObject != 'undefined'){
try{
return new ActiveXObject('Microsoft.XMLHTTP');
}catch(e){ return false; }
}else{
return false;
}
})(),

encodeURIParams = function(params){
var url = [], key, i;

for(key in params){
//in the case of a select multiple
if(typeof params[key] == 'object'){
for(i = params[key].length; i--{
url.push(encodeURIComponent(key), '=',
encodeURIComponent(params[key]), '&');
}

}else{
url.push(encodeURIComponent(key), '=',
encodeURIComponent(params[key]), '&');
}
}
return url.join('');
},

Request = function(url, params, onsuccess, method){
var xmlHttp = getXhr,
post = null;

xmlHttp.onreadystatechange = function(){
if(onsuccess && xmlHttp.readyState==4){
onsuccess(xmlHttp.responseText, xmlHttp.responseXML);

xmlHttp.onreadystatechange = function(){};
}
};

if(method && method.toUpperCase() == 'POST'){
post = encodeURIParams(params);
xmlHttp.open(method, url, true);
xmlHttp.setRequestHeader('Content-type', 'application/x-
www-form-urlencoded');
}else{
method = 'GET';
//need to deal with uris with existing query
xmlHttp.open(method, url+'?'+encodeURIParams(params),
true);
}

try{
xmlHttp.send(post);
}catch(e){}
};

return { Request : Request };
})();

 

[Asen Bozhilov]

        xmlHttp.onreadystatechange = function(){
                xmlHttp.onreadystatechange = function(){};

That isn't break circular reference in the way you do it. You again
form closure and you produce long Scope Chain. That empty function
refer from [[scope]] property AO/VO associated with execution context
in wich you create.

AO/VO => Request EC
^
AO/VO => xmlHttp.onreadystatechange EC
^
[[scope]] of xmlHttp.onreadystatechange = function(){};

Read that article <URL: http://www.jibbering.com/faq/faq_notes/closures.html>

 

[David Mark]

That isn't break circular reference in the way you do it. You again
form closure and you produce long Scope Chain.

Correct.

var fn = function() {};

....

(function() {

....

xmlHttp.onreadystatechange = fn; // fn defined outside of closure