JavaScript ajax library critique

[JR]

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();

As (window.someThing) can return an object, null or undefined (on the
browser-side), it's irrelevant to check for 'typeof window.someThing'.
So,

if (typeof window.XMLHttpRequest != 'undefined') {}

is equivalent to just

if (window.XMLHttpRequest) {} // null, undefined and '' evaluate to
false.

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

Firstly, lose that "return false" in the catch(e) { } block. Secondly,
you've got to try "Msxml2.XMLHTTP" too, not just "Microsoft.XMLHTTP".
I suggest checking for that in the first place, because IE is *still*
the most used browser (I hat to admit that). For instance:

var getXhr = (function() {
var activeXmodes = ["Msxml2.XMLHTTP", "Microsoft.XMLHTTP"];
if (window.ActiveXObject) {
for (var i = 0; i < activeXmodes.length; i++) {
try { return new ActiveXObject(activeXmodes); }
catch (e) { } // Ignores any error.
}
} else if (window.XMLHttpRequest) { // Firefox, Opera 8.0+, Safari.
return new XMLHttpRequest();
} else {
// this browser doesn't support AJAX.
return false;
}
})(),

 

[David Mark]

As (window.someThing) can return an object, null or undefined (on the
browser-side), it's irrelevant to check for 'typeof window.someThing'.
So,

  if (typeof window.XMLHttpRequest != 'undefined') {}

is equivalent to just

  if (window.XMLHttpRequest) {} // null, undefined and '' evaluate to
false.

Never detect host objects by type conversion. Some of them blow
up.

Firstly, lose that "return false" in the catch(e) { } block. Secondly,
you've got to try "Msxml2.XMLHTTP" too, not just "Microsoft.XMLHTTP".

As noted.

I suggest checking for that in the first place, because IE is *still*
the most used browser (I hat to admit that). For instance:

var getXhr = (function() {
  var activeXmodes = ["Msxml2.XMLHTTP", "Microsoft.XMLHTTP"];
  if (window.ActiveXObject) {

No. IE has supported XMLHttpRequest since version 7, some users have
ActiveX disabled for them and some agents have dummy global
ActiveXObject properties to prevent unnecessary exclusion by browser
sniffing scripts.

 

[Asen Bozhilov]

var fn = function() {};

...

(function() {

...

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

Excellent post. I hope OP understand your post. If you wrote:

(function(){
var fn = function() {};

Circular reference will be still there.

getXhr -> onreadystatechange -> fn -> AO -> getXhr

I have only one question. Why you suggest here to use function
expression and resolve `fn' identifier form Scope Chain instead of
`Function' constructor to create object with [[scope]] property which
refer Global Object? He can do it:

xmlHttp.onreadystatechange = Function();

 

[Mychal Hackman]

Excellent post. I hope OP understand your post. If you wrote: Agreed.

(function(){
  var fn = function() {};

Circular reference will be still there.

Yes because the function is created within the scope of
xmlHttp.onreadystatechange. If I understand correctly, the situation
would be exactly the same as before.

 

[Asen Bozhilov]

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

Why you mixin `11.2.1 Property Accessors` and `11.1.2 Identifier
Reference`? That can be very hard to debug. See below:

(function(){
var XMLHttpRequest;
if (window.XMLHttpRequest)
{
new XMLHttpRequest(); //TypeError for 11.2.2 point 3
}
}());

Should be:

new window.XMLHttpRequest();

onsuccess(xmlHttp.responseText, xmlHttp.responseXML);

onsuccess should be test for internal [[Call]] before you called.

if (typeof onsuccess == 'function')
{
onsuccess();
}

 

[David Mark]

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

Why you mixin `11.2.1 Property Accessors` and `11.1.2 Identifier
Reference`? That can be very hard to debug. See below:

(function(){
  var XMLHttpRequest;
  if (window.XMLHttpRequest)
  {
    new XMLHttpRequest(); //TypeError for 11.2.2 point 3
  }

}());

Should be:

new window.XMLHttpRequest();

onsuccess(xmlHttp.responseText, xmlHttp.responseXML);

onsuccess should be test for internal [[Call]] before you called.

if (typeof onsuccess == 'function')

Overkill. Documentation should specify the types of expected
arguments.

 

[JR]

Never detect host objects by type conversion.  Some of them blow
up.  

I can't remember a single case in which window.XMLHttpRequest or
window.ActiveXObject should 'blow up'. I'm sitting on a bomb and
didn't know about that (just kidding).

Firstly, lose that "return false" in the catch(e) { } block. Secondly,
you've got to try "Msxml2.XMLHTTP" too, not just "Microsoft.XMLHTTP".

As noted.

I suggest checking for that in the first place, because IE is *still*
the most used browser (I hat to admit that). For instance:

var getXhr = (function() {
  var activeXmodes = ["Msxml2.XMLHTTP", "Microsoft.XMLHTTP"];
  if (window.ActiveXObject) {

No.  IE has supported XMLHttpRequest since version 7

Ok, I forgot that IE's native XMLHTTP support is enabled by default.
But anyone can disable it, so... Anyway, if even MS checks for the
native object as a first step, I won't be the 'smart alec' here saying
the contrary:

http://msdn.microsoft.com/en-us/library/ms537505(VS.85).aspx

[...] some users have
ActiveX disabled for them and some agents have dummy global
ActiveXObject properties to prevent unnecessary exclusion by browser
sniffing scripts.

I didn't follow what's the problem?

 

[Garrett Smith]

var fn = function() {};

[...]

I have only one question. Why you suggest here to use function
expression and resolve `fn' identifier form Scope Chain instead of
`Function' constructor to create object with [[scope]] property which
refer Global Object? He can do it:

xmlHttp.onreadystatechange = Function();

The built-in Function constructor uses much less efficient algorithm for
building a function. A straight function expression is much simpler.

xmlHttp.onreadystatechange = noop;

If you get tired of defining a noop in all your modules, you want to
reuse a global function with global scope.

Function.prototype is a globally accessible function with global scope.
Function.prototype implements [[Call]] but not [[Construct]].

 

[Asen Bozhilov]

Never detect host objects by type conversion.  Some of them blow
up.  

Yes, i agree with you especially in ToPrimitive. But for ToBoolean are
have any example where ToBoolean can be harmful. I think ToBoolean and
ToObject, check the internal type of object.

ToBoolean:
if (Type(value) === Object)
{
return true;
}

ToObject:
if (Type(value) === Object)
{
return value;
}

 

[David Mark]

var fn = function() {};

(function() {

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

Excellent post. I hope OP understand your post. If you wrote:

(function(){
  var fn = function() {};

Circular reference will be still there.

getXhr -> onreadystatechange -> fn -> AO -> getXhr

I have only one question. Why you suggest here to use function
expression and resolve `fn' identifier form Scope Chain instead of
`Function' constructor to create object with [[scope]] property which
refer Global Object? He can do it:

xmlHttp.onreadystatechange = Function();

The Function constructor is inherently evil (avoid whenever
possible). Calling it without the new Operator is bad form. Creating
a new function each time through is inefficient.

 

[Asen Bozhilov]

Overkill.  Documentation should specify the types of expected
arguments.

You are absolutely right. And you use absolutely right term. Thanks
for correction

 

[David Mark]

I can't remember a single case in which window.XMLHttpRequest or
window.ActiveXObject should 'blow up'. I'm sitting on a bomb and
didn't know about that (just kidding).

Do you keep a running list of host objects that do? It's best to
avoid the issue.

As noted.

I suggest checking for that in the first place, because IE is *still*
the most used browser (I hat to admit that). For instance:
var getXhr = (function() {
  var activeXmodes = ["Msxml2.XMLHTTP", "Microsoft.XMLHTTP"];
  if (window.ActiveXObject) {

No.  IE has supported XMLHttpRequest since version 7

Ok, I forgot that IE's native XMLHTTP support is enabled by default.
But anyone can disable it, so...

You need to use a try-catch with it as well.

Anyway, if even MS checks for the
native object as a first step, I won't be the 'smart alec' here saying
the contrary:
Huh?


http://msdn.microsoft.com/en-us/library/ms537505(VS.85).aspx

What is it you see there?

[...] some users have
ActiveX disabled for them and some agents have dummy global
ActiveXObject properties to prevent unnecessary exclusion by browser
sniffing scripts.

I didn't follow what's the problem?

Some agents implement window.ActiveXObject, but it doesn't do anything.

 

[Asen Bozhilov]

Overkill.  Documentation should specify the types of expected
arguments.

Again here you are correct, but if it that argument is optional, i
think must be tested for [[Call]] before called. How you think about
that case and design with optiona* arguments?

 

[David Mark]

Yes, i agree with you especially in ToPrimitive. But for ToBoolean are
have any example where ToBoolean can be harmful. I think ToBoolean and
ToObject, check the internal type of object.

Yes. There are lots of examples (e.g. all ActiveX methods):-

if (window.external.addFavorite) {

....and properties under some circumstances:-

if (el.offsetParent) {

....will also blow up. The indicator is a typeof result of "unknown".

 

[Asen Bozhilov]

Yes.  There are lots of examples (e.g. all ActiveX methods):-

No. From that i can see they have own implementation of internal
[[Get]] and [[Put]] methods.

var a = window.external.addFavorite;

No type conversion but error is still there. Interesting in that part
is typeof.

| 1. Evaluate UnaryExpression.
| [...]
| 4. Call GetValue(Result(1))

So here again we have implicit invocation of [[Get]] method of that
`object'. But i m not sure people of Microsoft implement in that way.
How you think about that?

 

[David Mark]

Yes.  There are lots of examples (e.g. all ActiveX methods):-

No. From that i can see they have own implementation of internal
[[Get]] and [[Put]] methods.

var a = window.external.addFavorite;

No type conversion but error is still there. Interesting in that part
is typeof.

| 1. Evaluate UnaryExpression.
| [...]
| 4. Call GetValue(Result(1))

So here again we have implicit invocation of [[Get]] method of that
`object'. But i m not sure people of Microsoft implement in that way.
How you think about that?

I think they are nuts, but host objects can be implemented any way the
developers see fit.

I detect features with isHostMethod/isHostObjectProperty so I don't
have to keep track of their nonsense (and so I have a single line of
code to update in case they decide to change the rules after ten
years).

 

[Garrett Smith]

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.

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

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.

You seem to have a misunderstanding of how scope chain and identifier
resolution works.

The next object in scope chain of the function assigned in
onreadystatechange is the function object assigned to Ajax.Request the
next object in the scope chain after that is the containing scope, which
contains the Ajax object itself.

onreadystatechange handler Scope --> Ajax.Request Scope --> containing
scope (global?)

In fact, Ajax object does not have to be global at all.

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

Ah, not that is you failing to see the relevance, not me writing
irrelevance.

The Ajax.transport is in scope of the function assigned. If, for
example, the anonymous function body set xhr = null, then there would be
still one remaining link to the XMLHttpRequest object. That reference is
Ajax.transport.
[Global Scope]
Variable: Ajax
|
|
[Ajax.Request Scope]
Variable: xhr, res, post, url, params, onsuccess, method
|
|
[onreadystatechange handler Scope]
Variable: {empty}

Start from the bottom of the diagram, in onreadystatechange handler
Scope. The scope chain includes Ajax.Request Scope, and then the scope
above that (global, apparently, but it doesn't really matter). The last
object on the scope chain has a reference to Ajax.transport.
Ajax.transport is in the scope chain of onreadystatechange.

 

[David Mark]

David Mark wrote:
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.
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).
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.

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.  

Of course it does.  Diagram it.

You seem to have a misunderstanding of how scope chain and identifier
resolution works.

No, you seem to be confused about how circular references are
created.

[...]

Ah, not that is you failing to see the relevance, not me writing
irrelevance.

No, you are just confusing the uninitiated with your confusion.

The Ajax.transport is in scope of the function assigned. If, for
example, the anonymous function body set xhr = null, then there would be
still one remaining link to the XMLHttpRequest object. That reference is
Ajax.transport.
[Global Scope]
Variable: Ajax
   |
   |
[Ajax.Request Scope]
   Variable: xhr, res, post, url, params, onsuccess, method
   |
   |
[onreadystatechange handler Scope]
   Variable: {empty}

Start from the bottom of the diagram, in onreadystatechange handler
Scope. The scope chain includes Ajax.Request Scope, and then the scope
above that (global, apparently, but it doesn't really matter). The last
object on the scope chain has a reference to Ajax.transport.
Ajax.transport is in the scope chain of onreadystatechange.

You are missing a link in the chain (Activation Object). Without it,
there's no linkage between the Function object and the globally
declared Ajax object.

 

[Garrett Smith]

David Mark wrote:
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:

[...]

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'.

So what? They removed the obvious.

A legal implementation extension might be to add behavior to
Function.prototype that causes system side effects. The same is true for
any built-in function (parseInt, etc).

 

[Garrett Smith]

David Mark wrote:
Mychal Hackman wrote:
[...]

[...]


No, you are just confusing the uninitiated with your confusion.

You wrote:


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

The Ajax object is not a member of the containing scope, but it is a
member of an object in the containing scope's containing scope.

So rather than xhr -> onreadystatechange function [[Scope]] -> function
[[Scope]] -> xhr, we have:

xhr -> onreadystatechange function [[Scope]] -> Ajax.Request function
[[Scope]] --> global [[Scope]].

You are missing a link in the chain (Activation Object). Without it,
there's no linkage between the Function object and the globally
declared Ajax object.

The variable/activation object is written as "Variable:". In the
onreadystatechange handler, it is empty, written as "(empty)".