I was prompted to do a code review on PrototypeJS. Here is a partial review.
Ten Issues
- Modification of Host Object - Non standard, doesn't work the same in all browsers. Don't do it!
- Class.Methods.addMethods -
- Don't rely on a Function's
toString!
- Don't introduce special behavior based on function Decompilation!
- The mimicing of class based inheritance through closures to get
$super
to work requires a significantly augmented scope chain.
- The Dollar Function - Meaningless and extremely inefficient
-
Element.Methods.visible - misnamed, non-reflective, and buggy/unreliable!
-
More Modifications to Host objects
-
Object.extend - Don't forget the JScript DontEnum bug!
-
$A - Relying on object's toString -
-
readAttribute - Don't rely on Function Decompilation
-
Ajax.Request - Add an Underscore?
-
Broken unescaping of HTML Strings
-
Unnecessary use of
with statement
-
Enumerable.include - Strict Equality or 2n with Loose Equality
-
getDimensions - Don't Expect clientWidth to be non-zero in IE
-
cumulativeOffset - doesn't account for borders, scroll offsets, or magic BODY.
Okay, so it's more than 10, and could be even more, but I have to stop somewhere!
Modification of Host Object
A Host object is an object provided by the Host environment, e.g. document, event, an element.
PrototypeJS relies on modifying Host objects as a part of its core approach. There are several
problems with this approach.
-
There is
no requirement in the EcmaScript specification that requires Host objects to be
modifiable (although they usually are).
-
There is no requirement in the EcmaScript specification that the Host
environment expose for its Host objects
a constructor with an associated prototype.
-
There is no requirement in the EcmaScript specification that objects expose
a
__proto__ property.
-
There is no guarantee by any standard that there will be a
__proto__ property present for an Element or what value, if any, it will hold.
The fact that Mozilla exposes the prototypes of Element, et c is useful in that it allows
developers to provide quick patches for new or broken features that lack proper support.
However, this feature can't be reliably used for websites that are expected to run in multiple browsers.
Class.Methods.addMethods
Class.Methods.addMethods relies on the approach of calling toString on a function,
expecting to get back the source code, as it originally appeared.
if(value.argumentNames().first() == "$super")
A few significant problems:
-
Function.prototype.toString is not required to return the source code of the function
Function.prototype.toString is guaranteed only to return an implementation-dependent
representation of the function as a FunctionDeclaration [or FunctionExpression] (spec errata, has been fixed).
(§15.3.4.2).
Opera mobile
follows the spec
but takes the liberty of not returning the source code of the function.
-
Even if relying on the name of a function's formal parameter were guaranteed,
doing so would make implementation code fragile because it would be impossible for a human
(or compressor) to rename it (YUI Compressor bug).
Object.extend(Function.prototype, {
argumentNames: function() {
var names = this.toString().match(/^[\s\(]*function[^(]*\((.*?)\)/)[1].split(",").invoke("strip");
return names.length == 1 && !names[0] ? [] : names;
},
Providing a toString that returns helpful debugging info
for objects is generally good advice.
However, the return value of a function's toString cannot be expected to be anything
other than a string value. It should not be relied on.
Unfortunately
argumentNames will choke on a
Function object that has a custom toString. This problem will happen in every browser.
Example: Programmer-defined toString Causes Problem with Object.extend
function WidgetFactory(){}
WidgetFactory.toString = function(){ return"[WidgetFactory constructor]"; };
If argumentNames() is called on WidgetFactory, the first call to match() will return null. Then, when
split() is called, a TypeError will be thrown.
It would be less error-prone for PrototypeJS to use:
var names = Function.prototype.toString.call(someFunction);
This avoids the mistake of relying on the function instance's toString,
however, the approach still has two significant problems:
- relies on the source code and named formal
parameters of the function on which it is called (problem 1).
-
Function.prototype.toString is still not required to return the source code of the function
Function.prototype.toString is guaranteed to return an implementation-dependent
representation of the function as a FunctionDeclaration [or FunctionExpression] (spec errata).
(§15.3.4.2),
and in fact, Opera mobile
follows the spec correctly,
but takes the liberty of not returning the source code of the function.
Relying on Function Objects' toString
Do Not Rely on Return Values from Function Objects' toString!
In general, toString should not be parsed or relied upon for data formats.
toString is useful for debugging. In October 2007, I
pointed out how Dojo and jQuery made this mistake. Hallvord has been writing against relying on Function Decompilation for
well over a year, as it resulted in problems in PrototypeJS and jQuery running in Opera Mobile.
The Dollar Function
PrototypeJS is built on the approach of modifying Host objects. This is the cornerstone of the library's problems.
The library will try to modify the built-in prototype of an XPConnect wrapped prototype if it assumes
the browser can do that. Otherwise, it will add properties to the element itself.
The primary accessor to these modified Host objects is through that dollar function.
Law of Demeter
Each unit should have only limited knowledge about other units: only units "closely" related to the current unit.
(LoD).
I'll come back to explain how Prototype.js' modifications to Host objects violate LoD, and the problems
associated with that.
What Does $ Do?
-
$ does not have a clearly defined meaning as to what the function actually does.
-
The dollar sign is intended to be reserved for machine-generated code.
PrototypeJS $ function gets an element or array of elements. Calling $ results in a bare minimum of three function calls: $, isString, and Element.extend and a maximum of over 135 function calls.
function $(element) {
if (arguments.length > 1) {
for (var i = 0, elements = [], length = arguments.length; i < length; i++)
elements.push($(arguments[i]));
return elements;
}
if (Object.isString(element))
element = document.getElementById(element);
return Element.extend(element);
}
Call Stack of $ (best case)
$ -> isString
-> document.getElementById
-> Element.extend
Count of Functions from $ (best case)
$................................+1
+--isString.....................+1
+--document.getElementById.....(+1) (when isString(element) is true)
+--Element.extend...............+1
~--Prototype.K||(anonymous)...0 (alias, second time, only. Prototype.K only if SpecificElementExtensions)
Sub-total_________________________3 (4 when isString(element) is true).
Calling $(document) then $(document), there will be many function calls the first time, and never any fewer than three
calls the second time.
Element.extend = (function() {
if (Prototype.BrowserFeatures.SpecificElementExtensions)
return Prototype.K;
var Methods = { }, ByTag = Element.Methods.ByTag;
Object.extend(function(element) {
if (!element || element._extendedByPrototype ||
element.nodeType != 1 || element == window) return element;
var methods = Object.clone(Methods),
tagName = element.tagName, property, value;
// extend methods for specific tags
if (ByTag[tagName]) Object.extend(methods, ByTag[tagName]);
for (property in methods) {
value = methods[property];
if (Object.isFunction(value) && !(property in element))
element[property] = value.methodize();
}
element._extendedByPrototype = Prototype.emptyFunction;
return element;
}, {
refresh: function() {
// extend methods for all tags (Safari doesn't need this)
if (!Prototype.BrowserFeatures.ElementExtensions) {
Object.extend(Methods, Element.Methods);
Object.extend(Methods, Element.Methods.Simulated);
}
}
extend.refresh();
return extend;
})();
Element.extend, first call
However, during the first call to $(document), there is a check to Prototype.BrowserFeatures.SpecificElementExtensions.
This is done because when SpecificElementExtensions is true, then the Library attempts to add the properties to
DOM interface prototypes, e.g. HTMLHeadingElement.prototype. This happens elsewhere in the code, only when
a __proto__ property returns a truthy value on created elements. If this feature is supported, the author has
makes modifications to the DOM object interfaces, e.g. "HTML" + element.tagName + "Element", et c, but only
when the userAgent does not match /Apple.*Mobile.*Safari/. (He makes said modifications elsewhere in the code,
Element.addMethods -> findDOMClass).
Element.extend's closure calls Object.extend (needs review) to add the refresh()
method to element.extend (rather than perform a simple assignment).
The closure then calls extend.refresh() before returning the method extend,
which gets assigned to Element.extend.
+ Element.extend's closure -> Object.extend -> extend.refresh -> Object.extend...3
Object.extend...1
subtotal:_____________________________________________9
calls: 5, Depth: 3
These calls are done when the file is loaded. They don't affect performance of $.
After extend has been assigned to Element.extend, it is invoked.
Element.extend calls Object.clone, which calls Object.extend, then ByTag, Object.extend, n calls to isFunction,
n calls to methodize, where n is the number of properties of methods.
The element is then "methodized", which adds Ajax, et c to a FORM element.
$ -> isString..........................................(2)
document.getElementById...........................(+1) (when isString(element) is true)
Element.extend -> Object.clone -> Object.extend..(2)
ByTag -> Object.extend.........(2) (depends on tagName).
isFunction.....................(n = Size(methods) = 64 + 8)
methodize......................(n = Size(methods) = 64 + 8)
Total:_________________________________________________151
Calling $("sp-searchtext") results in 151 function calls the first time it is called.
Non-form elements will have 133 calls the first time.
This happens for every object that is got by dollar function for browsers that don't
support modifying DOM prototypes.
It considerably more complex and inefficient the first time around, when SpecificElementExtensions is false:
Object.extend(Methods, Element.Methods);
Object.extend(Methods, Element.Methods.Simulated);
Where is $ Used Internally?
All of the Element.methods of PrototypeJS functions (the ones that got "methodized" in dollar,
use the dollar function, as well, adding extra overhead of a function call.
This allows each of Element.methods to be used as a static method. This also adds considerable extra
cost, though, to each call.
Element.Methods = {
// (GS) visibility is not display!
visible: function(element) {
// (GS) style properties do not reflect element state.
// The value 'inherit' is not considered.
return $(element).style.display != 'none';
},
Element.Methods.visible does not deal with CSS visibility, but instead
returns the style.display != 'none'. The
display property does not reflect the currentStyle or computed style.
Also, the CSS
display property
can have the value inherit.
Other Element.Methods functions that use the dollar function are more expensive. For example:
// (GS) Avoid this long chains of calls. Hard to debug.
// (GS) calling Element.extend on each element is expensive,
// up to 1600+ function calls for a FORM with 10 elements in IE.
descendants: function(element) {
return $A($(element).getElementsByTagName('*')).each(Element.extend);
},
...
var Enumerable = {
each: function(iterator, context) {
var index = 0;
iterator = iterator.bind(context);
try {
this._each(function(value) {
iterator(value, index++);
});
} catch (e) {
if (e != $break) throw e;
}
return this;
},
...
Object.extend(Array.prototype, {
_each: function(iterator) {
for (var i = 0, length = this.length; i < length; i++)
iterator(this[i]);
},
Considering a FORM with 10 input elements and nothing else,
there will be:
Element.descendants...................1
$A-->Array........................2
+--$.............................151
+--getElementsByTagName('*')..1
each.............................1 x 10 (ten INPUT elements)
+--bind.........................1 x 10
+--_each........................151 x 10 (151 methods, ten INPUT elements)
TOTAL.................................1685
One thousand, six hundred, and eighty-five function calls.
The call to isFunction in Element.extend might be something that could be refactored:
value = methods[property];
Object.isFunction(value)
Variable methods is a collection of functions.
Calling Object.isFunction(value) should always return true here and this is something
that the author can have control over because he owns methods (Law of Demeter).
Always Use Dollar
When using PrototypeJS, it is uncertain what properties will be present on an Element. This is because
PrototypeJS may have already modified that element or its associated prototype. This dilemma of ambiguity can
be avoided by the PrototypeJS user always using the dollar function and depending on PrototypeJS.
$("adiv").parentNode.show(), will have unexpected results, and will result in error in IE,
if the parentNode has not been previously modified via Element.extend()
$($('adiv').parentNode), will result in no less than seven function calls, and that is only after
$('adiv') has been called and
$($('adiv').parentNode) has been called.
More Modifications to Host Objects
I'm going to back up a little bit and look at details I skipped over, the modification Host objects.
LoD Recap
If the implementation's
internal code undergoes change, or if other browsers provide similar properties with
slightly different implementation then the code that relies on
assumptions of implementation-specific details based on those properties (col.__proto__)
will fail. Implementations have been known to change, in this regard
(bug 390411)
function findDOMClass(tagName) {
var klass;
var trans = {
"OPTGROUP": "OptGroup", "TEXTAREA": "TextArea", "P": "Paragraph",
// (GS) et c...
};
if (trans[tagName]) klass = 'HTML' + trans[tagName] + 'Element';
// (GS) Does not provide any feature detection about the object.
if (window[klass]) return window[klass];
klass = 'HTML' + tagName + 'Element';
if (window[klass]) return window[klass];
klass = 'HTML' + tagName.capitalize() + 'Element';
if (window[klass]) return window[klass];
window[klass] = { };
// (GS) Does not provide any feature detection about the object.
window[klass].prototype = document.createElement(tagName).__proto__;
return window[klass];
}
if (F.ElementExtensions) {
copy(Element.Methods, HTMLElement.prototype);
copy(Element.Methods.Simulated, HTMLElement.prototype, true);
}
if (F.SpecificElementExtensions) {
for (var tag in Element.Methods.ByTag) {
var klass = findDOMClass(tag);
if (Object.isUndefined(klass)) continue;
copy(T[tag], klass.prototype);
}
}
The findDOMClass assumes that the window will have an object based on tagName
or that if this is not the case, then the element's __proto__ property will be readable.
There is no guarantee by any standard that there will be a __proto__
property present or what value it will hold, if any.
There is no guarantee by any standard of an HTMLTableRowElement on window, nor any guarantee of what modifying its prototype will have. The library makes broad assumptions and performs no capability tests.
Replace the Host Environment's Element?
Not satisfied by altering over certain Host environments' Element
with its own properties,
PrototypeJS seeks to create a constructor to replace those environments' Element
with its own, copying over all enumerable properties from the Host's Element.
In Firefox, this includes all of QueryInterface.
This creates the dilemma of having a debilitated Element. Where previously, in
Firefox,
Element was native code, and Element.prototype.TEXT_NODE had the value
3, now, Element is a PrototypeJS constructor function and
Element.prototype.TEXT_NODE is undefined.
(function() {
var element = this.Element;
this.Element = function(tagName, attributes) {
attributes = attributes || { };
tagName = tagName.toLowerCase();
var cache = Element.cache;
if (Prototype.Browser.IE && attributes.name) {
tagName = '<' + tagName + ' name="' + attributes.name + '">';
delete attributes.name;
return Element.writeAttribute(document.createElement(tagName), attributes);
}
if (!cache[tagName]) cache[tagName] = Element.extend(document.createElement(tagName));
return Element.writeAttribute(cache[tagName].cloneNode(false), attributes);
};
Object.extend(this.Element, element || { });
}).call(window);
Element.cache = { };
Event.extend - Don't do It!
The same approach is used here
Event.extend = (function() {
var methods = Object.keys(Event.Methods).inject({ }, function(m, name) {
m[name] = Event.Methods[name].methodize();
return m;
});
if (Prototype.Browser.IE) {
Object.extend(methods, {
stopPropagation: function() { this.cancelBubble = true },
preventDefault: function() { this.returnValue = false },
inspect: function() { return "[object Event]" }
});
return function(event) {
if (!event) return false;
if (event._extendedByPrototype) return event;
event._extendedByPrototype = Prototype.emptyFunction;
var pointer = Event.pointer(event);
Object.extend(event, {
target: event.srcElement,
relatedTarget: Event.relatedTarget(event),
pageX: pointer.x,
pageY: pointer.y
});
return Object.extend(event, methods);
};
} else {
// (GS) Event.prototype is not guaranteed to be available in
// any browser.
Event.prototype = Event.prototype || document.createEvent("HTMLEvents").__proto__;
Object.extend(Event.prototype, methods);
return Prototype.K;
}
})();
Object.extend - Doesn't Account for JScript DontEnum Bug
Object.extend = function(destination, source) {
for (var property in source)
// (GS) Does not account for JScript DontEnum bug.
destination[property] = source[property];
return destination;
};
In IE, the keys of objects are skipped
without properly checking the DontEnum
flag. The skipped properties include
the useful and often overridden toString and valueOf.
Special and careful considerations should be made to address this problem.
$A - Don't Rely on The Return Value of An Object's toString
if (Prototype.Browser.WebKit) {
$A = function(iterable) {
if (!iterable) return [];
// (GS) Do not on the return value of an object's toString.
if (!(Object.isFunction(iterable) && iterable == '[object NodeList]') &&
iterable.toArray) return iterable.toArray();
var length = iterable.length || 0, results = new Array(length);
while (length--) results[length] = iterable[length];
return results;
};
}
A toString() with the return
value of "[object NodeList]" implies nothing else about the object
it was called on.
The readAttribute function, branched for Internet Explorer.
Example
Example page: http://www.apple.com/itunes/
javascript:alert($(document.body).readAttribute("onload"))
Will result in partial string representation of a serialized function.
The string begins with ")".
_getEv: function(element, attribute) {
// (GS) Do not rely on function decompilation.
var attribute = element.getAttribute(attribute);
// (GS) Do not prevent yourself from renaming a function.
// (GS) Broken - slice starts at wrong position.
return attribute ? attribute.toString().slice(23, -2) : null;
},
Function readAttribute calls getAttribute on the element. In Internet Explorer,
calling getAttribute always reflects the property with the same name.
In this case the property has the value of the function object, loadShortcuts.
It isn't clear why the substring of 23 should be taken from the function's source code.
The approach
of relying on the toString of an object defined elsewhere in the code
makes it hard to rename that function.
Ajax.Request - Add an Underscore?
request: function(url) {
this.url = url;
this.method = this.options.method;
var params = Object.clone(this.options.parameters);
if (!['get', 'post'].include(this.method)) {
// simulate other verbs over post
params['_method'] = this.method;
this.method = 'post';
}
this.parameters = params;
if (params = Object.toQueryString(params)) {
// when GET, append parameters to URL
if (this.method == 'get')
this.url += (this.url.include('?') ? '&' : '?') + params;
// (GS) All browsers support properly-formatted URIs.
// (GS) Do not add extra "_" parameter for some browsers.
else if (/Konqueror|Safari|KHTML/.test(navigator.userAgent))
params += '&_=';
}
Broken Unescaping of HTML Strings
PrototypeJS adds escapeHTML and unescapeHTML to String.prototype.
The problem with this code was discussed quite some time ago on
comp.lang.javascript.
if (Prototype.Browser.WebKit || Prototype.Browser.IE) Object.extend(String.prototype, {
escapeHTML: function() {
return this.replace(/&/g,'&').replace(/</g,'<').replace(/>/g,'>');
},
unescapeHTML: function() {
return this.replace(/&/g,'&').replace(/</g,'<').replace(/>/g,'>');
}
});
// (GS) Do not use with for simple property assignment
with (String.prototype.escapeHTML) div.appendChild(text);
The problem is that the escape character for entities is &. This will have the result in
browsers identifying as Webkit or MSIE of:-
"&lt;".unescapeHTML() to "<"
The with statement is not needed to provide a more compact approach:
"".escapeHTML.div.appendChild("".escapeHTML.text);
Enumerable.include - Two Loops? Strict Equality, or Loose Equality?
Enumerable.include. This function returns true if the Enumerable contains the object.
include: function(object) {
// (GS) Call the indexOf method on the object.
if (Object.isFunction(this.indexOf))
if (this.indexOf(object) != -1) return true;
// (GS) If not found, search again.
var found = false;
this.each(function(value) {
// (GS) Should use strict equality, ===
if (value == object) {
found = true;
throw $break;
}
});
return found;
},
Enumerable.include first calls this.indexOf(), and if that returns false, the collection
is searched using a similar algorithm to Array.prototype.indexOf, except not using
strict equality. This tactic results in the collection being looped over twice. Function
each calls a function for each iteration. If non-strict equality is desired, then the strict equality check
that can result by calling this.indexOf should be omitted.
A plausible solution would be return this.indexOf(object), and include a
strict === check if indexOf were not a function.
getDimensions - clientWidth != offsetWidth, And Don't Expect clientWidth to be non-zero in IE
getDimensions: function(element) {
element = $(element);
var display = $(element).getStyle('display');
if (display != 'none' && display != null) // Safari bug
return {width: element.offsetWidth, height: element.offsetHeight};
// All *Width and *Height properties give 0 on elements with display none,
// so enable the element temporarily
var els = element.style;
var originalVisibility = els.visibility;
var originalPosition = els.position;
var originalDisplay = els.display;
els.visibility = 'hidden';
els.position = 'absolute';
els.display = 'block';
var originalWidth = element.clientWidth;
var originalHeight = element.clientHeight;
els.display = originalDisplay;
els.position = originalPosition;
els.visibility = originalVisibility;
// (GS) use offsetWidth/Height.
return {width: originalWidth, height: originalHeight};
},
This method has the unique quality of being one of the only methods in PrototypeJS to
have a code comment.
The code tries to get the offsetWidth and offsetHeight
of the Element. If the element is not displayed,
then these properties will be 0, and in that case, the function will display the element temporarily,
then calculate its clientWidth - not the offsetWidth before immediately
hiding it. This function can be expected to
provide inconsistent results when the element's CSS display is changing.
What's worse: Unless the element has a CSS width, the clientWidth will be 0 in IE.
Calculating Offsets
Finding an Element's position is hard.
Calculating the position of an element requires adding the offsetTop/Left of
offsetParents, border offsets,
and scroll offsets of parentNodes.
A BODY with margin, position and/or border can affect the offsetTop/Left differently, depending
on the browser.
This is a harmful effect propagated by the
CSSOM Views standard.
cumulativeOffset
PrototypeJS misses both points and just adds offsetTop/Left.
cumulativeOffset: function(element) {
var valueT = 0, valueL = 0;
do {
valueT += element.offsetTop || 0;
valueL += element.offsetLeft || 0;
element = element.offsetParent;
} while (element);
// (GS) Don't call a function that returns an
// Array with top and left properties.
// Better just return an Array or an Object.
return Element._returnOffset(valueL, valueT);
},
The seriousness of the problem is apparent when precision is crucial and off by a pixel or more would be failure,
(e.g. make element to overlap another element exactly, activate a drop zone).
The bug also exists in positionedOffset and affects every function that uses
either function, including within, scrollTo, and all the functions
that call those functions, such as others found in Scriptaculous (dragdrop.js, effects.js).
Conclusion
A few core parts of PrototypeJS exhibiting some serious bugs.
There are many more issues, such as Hash,
but this entry is already getting excessively long.
PrototypeJS is designed in a non-standard way around the modification of host objects. The code
that exists is complicated and not very efficient. I cannot recommend using this library
for anything.
Technorati Tags:
JavaScript
Code Review
Prototype
Prototype.js