Was asked to look at this thing again; only reviewed the JS portion. Last I checked, the CSS was similarly reality-challenged. Graphics are nice, but are tantamount to the paint job on a used car. Under the hood, this thing is all junk. It's hard to imagine the innards of any Web or mobile device-based application would end up like this, which raises the question of whether the authors have ever written such applications (or any amount of meaningful JS).
/**
* @class Ext
* @singleton
There (still) are no "classes" or "singletons" in JS (starting off on the wrong foot by mangling JS terms).
*/
(function() {
var global = this,
objectPrototype = Object.prototype,
toString = objectPrototype.toString,
enumerables = true,
enumerablesTest = { toString: 1 },
emptyFn = function(){},
i;
It's obvious what they are setting up with the "enumerables" flag and accompanying object. The question is why.
if (typeof Ext === 'undefined') {
global.Ext = {};
}
Line #2 requires an Automatic F. What they wanted was a global variable, but they ended up with is a property of the Global Object. According the specifications and (and implementation history) there are enough differences between the two that care is required not to mix them up (as is done here).
First they forgot to declare the "Ext" variable. Simple as that; or, perhaps they just don't know what they are doing. The latter case would hardly be precedent setting. Dojo and similar projects have been making these same mistakes for years (and constantly pleading for somebody to "show them where it fails").
One simple case where forgetting to declare a global variable will cause a catastrophic failure in at least some versions/modes of Internet Explorer is to include the script in a document featuring this bit:
<div id="Ext"> ... </div>
...Now there is a property of the global object called "Ext" to contend with. Line #2 fails and so goes Sencha Touch, simply because their JS programmers couldn't be bothered to learn the language. Their excuse for such failings has always been that they don't care about... well, whatever browsers cause their scripts to fail. :(
Moving on.
Ext.global = global;
Yes, well it certainly is handy to provide a reference to the Global Object. Of course, in the global context, once can simply write this:
var global = this;
Why did they declare "global" in a local scope and then reference it from their global object? Because they think that having two global variables (as opposed to one) is going to cause a problem for other scripts. Perhaps it might, but the irony is that Ext/Sencha scripts are notorious for breaking other scripts in ways far harder to track down than a global variable called "global" (what else would it be but a reference to the Global Object?)
if (enumerables) {
enumerables = ['hasOwnProperty', 'valueOf', 'isPrototypeOf', 'propertyIsEnumerable',
'toLocaleString', 'toString', 'constructor'];
}
/**
* An array containing extra enumerables for old browsers
* @property {String[]}
*/
Ext.enumerables = enumerables;
There's the "answer" to the "enumerables" question. In the first review of this thing, they referenced "old browsers" to justify the inclusion of:
var undefined;
...or some such nonsense that supposedly aided the execution of the script in unspecified "old browsers".
Same thing here. There's a very narrow context (requiring iterating properties that shadow properties of Object.prototype) that exposed a bug in (for example) "old" Internet Explorer browsers (which are still very much an issue on the Web due to corporate users and compatibility modes in "new" Internet Explorer browsers). As it is known that this group doesn't "care" about (for example) IE 7, have to ask what this code is doing in here. Certainly, as a whole, this script is never going to do anything positive in IE 7 (or equivalent compatibility modes).
It's not that it's wrong to take care in feature testing, but this is certainly is a glaring contradiction. It's quite possible that this general-purpose monolith has stumbled into the very narrow context that exposes the bug (which illustrates the problems with general-purpose monoliths). But it's just as possible that they are writing code without understanding what it is doing or why. Certainly their code comments (and lack thereof) illuminate nothing about the issue (other than the cryptic reference to "old browsers") and that doesn't bode well for future maintenance of this script. Either they are being deliberately coy or they just don't know. Regardless, they don't detect or test much of anything in this script, so it is odd they pasted this (it's certainly not original code).
When writing browser scripts, it's a good idea to keep context in sight. For example, if your application does not need to shadow and iterate over "toString" or other built-in properties, then you don't have to worry about which browsers might screw up such operations or waste the user/agent's time testing for these exceptions.
/**
* Copies all the properties of config to the specified object.
* Note that if recursive merging and cloning without referencing the original objects / arrays is needed, use
* {@link Ext.Object#merge} instead.
* @param {Object} object The receiver of the properties
* @param {Object} config The source of the properties
* @param {Object} defaults A different object that will also be applied for default values
* @return {Object} returns obj
*/
Ext.apply = function(object, config, defaults) {
I asked an "Ext JS expert" about this one once, indicating that JS already has an "apply" method (of Function objects). They said it was basically the same (!) and then transposed the last two arguments in the explanation to match examples pulled from their code. Unfortunately, that meant that all of their code was calling this method wrong. :(
JFTR, this has nothing at all to do with Function.prototype.apply. The name is just confusing. Basically, it's a two-tiered "mixin".
if (defaults) {
Ext.apply(object, defaults);
}
if (object && config && typeof config === 'object') {
The first check is absolutely counter-productive. Silent failures only serve to confuse developers. If having a problem with developers calling this method with no arguments (or a "falsey" first argument), add this:
if (!object) {
throw new Error('Try passing an object reference to apply, genius!');
}
...and remove such "scaffolding" on release. Such code is only of use to developers and testers. There is no reason to send it to users. The same goes for unnecessary argument validation; it's just a waste of time and space.
The second and third checks are the same story. It would be much easier on developers if the method blew up immediately, rather than failing silently until somebody (tester or end-user) stumbles across seemingly inexplicable behavior, investigates, files a ticket, etc. The developers won't even know where to start at that point. On a large project, these sorts of mysteries tend to pile up in the support queue.
var i, j, k;
Yes, this is what you want to see. Note to Ext JS developers: that's you cue to thrown down the review in disgust and rave about "trolls". That way you will miss all of the important points that follow. ;)
for (i in config) {
object[i] = config[i];
}
No for-in filter.
if (enumerables) {
for (j = enumerables.length; j--;) {
k = enumerables[j];
if (config.hasOwnProperty(k)) {
object[k] = config[k];
}
}
}
Been over that bit.
}
return object;
};
Then there's this:
/**
* Iterates either an array or an object. This method delegates to
...which is another automatic F (with extreme prejudice). This is another classic Dojo gaffe. If familiar with the language, you would not design a system that must differentiate between Array and Object objects. It's one of those language-imposed brick walls that experienced developers avoid like the plague.
* {@link Ext.Array#each Ext.Array.each} if the given value is iterable, and {@link Ext.Object#each Ext.Object.each} otherwise.
*
* @param {Object/Array} object The object or array to be iterated.
* @param {Function} fn The function to be called for each iteration. See and {@link Ext.Array#each Ext.Array.each} and
* {@link Ext.Object#each Ext.Object.each} for detailed lists of arguments passed to this function depending on the given object
* type that is being iterated.
* @param {Object} scope (Optional) The scope (`this` reference) in which the specified function is executed.
* Defaults to the object being iterated itself.
* @markdown
*/
iterate: function(object, fn, scope) {
Scope?! Use virtually any other word for the - this - object. I am partial to "thisObject".
if (Ext.isEmpty(object)) {
return;
}
if (scope === undefined) {
scope = object;
}
if (Ext.isIterable(object)) {
This "isIterable" function is beyond belief:
return (value && typeof value !== 'string') ? value.length !== undefined : false;
...and they are trying to use that to discriminate between Array and Object objects. It's a recurring pattern: paint into corner, use nonsense code to try to get out. Repeat until inevitable epiphanies hit every contributor on the project and then start designing a new version. The bigger the project, the more contributors there are, the longer the epiphanies take to trickle down to the design. Dinosaurs had relatively slow reaction times compared to mammals; these behemoths had brains that were too far from their tails. The same thing applies to large, monolithic JS projects. The only difference is that dinosaurs didn't have a marketing department. :)
Ext.Array.each.call(Ext.Array, object, fn, scope);
}
else {
Ext.Object.each.call(Ext.Object, object, fn, scope);
}
The ramifications of a miss here are left as an exercise (or you could just forget this silly script).
}
That's about enough.
Developers download this thing because they want to step all over touch gestures. The original review from a couple years back touched briefly on their gesture handling and it was all a bunch of UA sniffs, canceled bubbling and other assorted non-logic and bad practices. IIRC, it was originally a jQuery plug-in. :(
Then there was the endless JS to "control" layout, the ridiculous QSA wrapper (somehow they managed to make QSA worse than it is on its own). Here's the new version of "query":
query: function(selector, root) {
var selectors = selector.split(','),
length = selectors.length,
i = 0,
results = [],
noDupResults = [],
dupMatcher = {},
query, resultsLn, cmp;
for (; i < length; i++) {
selector = Ext.String.trim(selectors[i]);
query = this.parse(selector);
// query = this.cache[selector];
// if (!query) {
// this.cache[selector] = query = this.parse(selector);
// }
results = results.concat(query.execute(root));
}
// multiple selectors, potential to find duplicates
// lets filter them out.
if (length > 1) {
resultsLn = results.length;
for (i = 0; i < resultsLn; i++) {
cmp = results[i];
if (!dupMatcher[cmp.id]) {
noDupResults.push(cmp);
dupMatcher[cmp.id] = true;
}
}
results = noDupResults;
}
return results;
},
So they tried to fix one of the issues raised in the earlier review; unfortunately they also started work on a selector parser. They don't need a parser and they certainly can't filter duplicates by ID (unless they require every element in the document to have an ID). Actually, that rings a bell. I'll have to ask the aforementioned "expert" if they really require that. I do remember that the application had ID's on everything (ten tables deep in places) and I think I heard mention that it broke if they took one out. The other possibility is that the "results" array does not contain element references (but some sort of wrappers with their own ID schemes).
There's no doubt they are still using QSA (in some other function); it's just buried under more junk code (like the above "dupe" thing). Have to wonder if they even understand the limitations of QSA (if they do, they aren't telling). The host-provided querySelectorAll method is not hard to call. You can do bad all by yourself. :)
Here's a lower-level part of the query process:
/**
* @class Ext.DomQuery
* @alternateClassName Ext.dom.Query
*
* Provides functionality to select elements on the page based on a CSS selector. All selectors, attribute filters and
* pseudos below can be combined infinitely in any order. For example "div.foo:nth-child(odd)[@foo=bar].bar:first"
* would be a perfectly valid selector.
*
* ## Element Selectors:
*
* * \* any element
* * E an element with the tag E
* * E F All descendent elements of E that have the tag F
* * E > F or E/F all direct children elements of E that have the tag F
* * E + F all elements with the tag F that are immediately preceded by an element with the tag E
* * E ~ F all elements with the tag F that are preceded by a sibling element with the tag E
*
* ## Attribute Selectors:
*
* The use of @ and quotes are optional. For example, div[@foo='bar'] is also a valid attribute selector.
*
* * E[foo] has an attribute "foo"
* * E[foo=bar] has an attribute "foo" that equals "bar"
* * E[foo^=bar] has an attribute "foo" that starts with "bar"
* * E[foo$=bar] has an attribute "foo" that ends with "bar"
* * E[foo*=bar] has an attribute "foo" that contains the substring "bar"
* * E[foo%=2] has an attribute "foo" that is evenly divisible by 2
* * E[foo!=bar] has an attribute "foo" that does not equal "bar"
*
* ## Pseudo Classes:
*
* * E:first-child E is the first child of its parent
* * E:last-child E is the last child of its parent
* * E:nth-child(n) E is the nth child of its parent (1 based as per the spec)
* * E:nth-child(odd) E is an odd child of its parent
* * E:nth-child(even) E is an even child of its parent
* * E:only-child E is the only child of its parent
* * E:checked E is an element that is has a checked attribute that is true (e.g. a radio or checkbox)
* * E:first the first E in the resultset
* * E:last the last E in the resultset
* * E:nth(n) the nth E in the resultset (1 based)
* * E:odd shortcut for :nth-child(odd)
* * E:even shortcut for :nth-child(even)
* * E:contains(foo) E's innerHTML contains the substring "foo"
* * E:nodeValue(foo) E contains a textNode with a nodeValue that equals "foo"
* * E:not(S) an E element that does not match simple selector S
* * E:has(S) an E element that has a descendent that matches simple selector S
* * E:next(S) an E element whose next sibling matches simple selector S
* * E:prev(S) an E element whose previous sibling matches simple selector S
* * E:any(S1|S2|S2) an E element which matches any of the simple selectors S1, S2 or S3//\\
*
* ## CSS Value Selectors:
*
* * E{display=none} css value "display" that equals "none"
* * E{display^=none} css value "display" that starts with "none"
* * E{display$=none} css value "display" that ends with "none"
* * E{display*=none} css value "display" that contains the substring "none"
* * E{display%=2} css value "display" that is evenly divisible by 2
* * E{display!=none} css value "display" that does not equal "none"
*/
Those last half-dozen are ridiculous. Is that supposed to be the inline style or the computed style? Either way it is bad news. Now that we have a fairly standard query interface implemented in the majority of browsers in use, it makes no sense to start adding new selector types that will likely never be part of the standard. It's best to let the browser handle the queries; does away with the huge amount of script required to parse CSS selectors and perform queries. The way to do this is to narrow your focus (not widen it). There exists a "sane subset" of selector types that are consistent in IE 8+ and virtually every competing browser in use today. Many of the above selectors (and all of the made-up variety) are outside of this subset, resulting in the addition of lots of insane code. Like this bit:
Ext.define('Ext.dom.Query', {
/**
* Selects a group of elements.
* @param {String} selector The selector/xpath query (can be a comma separated list of selectors)
* @param {HTMLElement/String} [root] The start of the query (defaults to document).
* @return {HTMLElement[]} An Array of DOM elements which match the selector. If there are
* no matches, and empty Array is returned.
*/
select: function(q, root) {
var results = [],
nodes,
i,
j,
qlen,
nlen;
root = root || document;
if (typeof root == 'string') {
root = document.getElementById(root);
}
q = q.split(",");
Seen this before (in the original version). This is where they end up with lots of duplicates. What would you expect if you split a CSS selector on commas, process each chunk through QSA and concatenate the results. As seen above, at some point, all of the results get some sort of ID's. Doesn't really matter how or why; it just shows that this thing is needlessly complex, at least partly due to poor planning. They should have torn this up after the first review, but they've built more complexity on top of it.
One big problem with something like this (e.g. "Sizzle" and other QSA wrappers) is that some element-rooted queries (by far the most popular) don't work as Web developers have been conditioned to expect. The standard (Selectors API) didn't match the preexisting libraries and most of them never did anything about it, blissfully forking between QSA calls and incompatible junk like jQuery. At the time, most were swamped by IE 8 issues. :)
Ironically, given the penchant for ID's, the fix to change element-rooted queries to document-rooted queries is trivial.
for (i = 0,qlen = q.length; i < qlen; i++) {
if (typeof q[i] == 'string') {
//support for node attribute selection
if (q[i][0] == '@') {
nodes = root.getAttributeNode(q[i].substring(1));
results.push(nodes);
}
else {
nodes = root.querySelectorAll(q[i]);
for (j = 0,nlen = nodes.length; j < nlen; j++) {
results.push(nodes[j]);
}
}
}
}
return results;
},
/**
* Selects a single element.
* @param {String} selector The selector/xpath query
* @param {HTMLElement/String} [root] The start of the query (defaults to document).
* @return {HTMLElement} The DOM element which matched the selector.
*/
selectNode: function(q, root) {
return this.select(q, root)[0];
},
/**
* Returns true if the passed element(s) match the passed simple selector (e.g. div.some-class or span:first-child)
* @param {String/HTMLElement/Array} el An element id, element or array of elements
* @param {String} selector The simple selector to test
* @return {Boolean}
*/
is: function(el, q) {
if (typeof el == "string") {
el = document.getElementById(el);
}
return this.select(q).indexOf(el) !== -1;
Ridiculous assumption about Array.prototype.indexOf. ISTM they just cut out all known browsers with the property iteration bug. I could be wrong though; I don't normally keep track of or even consider such things until it is necessary (and I always avoid the iteration problem). Slightly less ridiculous assumption about gEBI. Easier to pin that one down, but why track of such things? That's what code is for.
},
isXml: function(el) {
var docEl = (el ? el.ownerDocument || el : 0).documentElement;
return docEl ? docEl.nodeName !== "HTML" : false;
}
Ridiculous on every level. The first line of code is not only poor feature detection logic, it's bad programming in any context or language. At a glance, it's gobbledygook. The second line makes an obviously invalid assumption, but to be fair and as there are no comments, the caller(s) should be looked at to determine what this thing is supposed to be doing. Turns out there's only one caller:
getRoot: function(data) {
var nodeName = data.nodeName,
root = this.getRootProperty();
if (!root || (nodeName && nodeName == root)) {
return data;
} else if (Ext.DomQuery.isXml(data)) {
// This fix ensures we have XML data
// Related to TreeStore calling getRoot with the root node, which isn't XML
// Probably should be resolved in TreeStore at some point
return Ext.DomQuery.selectNode(root, data);
}
},
I see. That fix ensures they have XML data (else it will fail silently). And something about TreeStore with the root node, which isn't XML. (?) And a FIXME. This is what I'm talking about. :(
Any time you sit down to write an "isXML" (or "isDocument", "isElement", etc.), stop and re-think your design as you can't make such assumptions about host objects. The idea of trying to straddle some imaginary line between HTML and XML DOM's in low-level DOM functions like this one is crazy. The idea is to keep such functions as simple as possible. Everything built on top of Sencha's "foundation" is suspect. In other words, you can't make any assumptions about how the code will behave in heretofore untested (or unknown) environments. You just don't know what they will do next. Scratch that, if you've followed Ext JS or Dojo or the dozen other similar "frameworks" out there, you know they are eminently predictable. They periodically blow up. They blow up real good. :)
},
If you want to do well with queries, then test the selectors your app needs to work before using them. Test only the ones you need; don't spend years trying to "fix" queries in general (you'll likely end up with dubious, outdated junk like the above).
Then there's this.
userAgent = Ext.browser.userAgent,
// These Android devices have a nasty bug which causes JavaScript timers to be completely frozen
// when the browser's viewport is being panned.
isBuggy = /(htc|desire|incredible|ADR6300)/i.test(userAgent) && version.lt('2.3');
...and this:
// Facebook changes the userAgent when you view a website within their iOS app. For some reason, the strip out information
// about the browser, so we have to detect that and fake it...
if (userAgent.match(/FB/) && browserName == "Other") {
browserName = browserNames.safari;
engineName = engineNames.webkit;
}
(Just one more reason why it pays to ignore the UA string.)
...And this:
prefixes: {
ios: 'i(?:Pad|Phone|Pod)(?:.*)CPU(?: iPhone)? OS ',
android: '(Android |HTC_|Silk/)', // Some HTC devices ship with an OSX userAgent by default,
// so we need to add a direct check for HTC_
blackberry: 'BlackBerry(?:.*)Version\/',
rimTablet: 'RIM Tablet OS ',
webos: '(?:webOS|hpwOS)\/',
bada: 'Bada\/'
}
...And this:
// This is here because some HTC android devices show an OSX Snow Leopard userAgent by default.
// And the Kindle Fire doesn't have any indicator of Android as the OS in its User Agent
if (match[1] && (match[1] == "HTC_" || match[1] == "Silk/")) {
version = new Ext.Version("2.3");
} else {
version = new Ext.Version(match[match.length - 1]);
}
...And this:
if (!osEnv.is.Android && !osEnv.is.iOS && /Windows|Linux|MacOS/.test(osName)) {
deviceType = 'Desktop';
}
else if (osEnv.is.iPad || osEnv.is.Android3 || (osEnv.is.Android4 && userAgent.search(/mobile/i) == -1)) {
deviceType = 'Tablet';
}
else {
deviceType = 'Phone';
}
...And can they really be serious with all of this BS? Any one of those is an automatic fail (since around 1999). The strategy was deemed unusable back when there were less than a half dozen browsers in use.
http://jibbering.com/faq/notes/detect-browser/
That article is from back around the turn of the century. To quote:
"We are now at a point where the contents of the User Agent strings bear no relationship at all to the capabilities and features of the browser that is reporting it. The situation has gone so far that a number of javascript experts have stated that a standard quality test for an unknown script would include searching the source code of the script for the string "userAgent" and dismissing the script out of hand if that string is found."
That's never been more true than this very instant and will be even more true tomorrow. Doing the opposite is not some sort of bold "pragmatic" strategy. It's just ignorant. In all my years of doing this stuff, I've never had a case where the arbitrary (and often indistinguishable) UA strings had to be considered. So no, they don't "have to" do any of the above nonsense and after another five years or so (judging by the progress between versions), perhaps they will have that epiphany (everyone does eventually).
Think your mobile users want to download a new version of this junk every other month? It's already huge and sure to grow every time. The authors clearly have no idea what it does, so the chances of botched releases are high. It's hard to release a huge blob of JS every other month and the way this thing is written and with new devices coming out every other week, it's to the point where they need to release new versions every hour.
Think of it as your company and users subsidizing the continuing education of Sencha's developers. You'll need continuing education too as they constantly change things around and then force you to "upgrade" due to their own invalid assumptions about UA strings. It's definitely a lose-lose proposition and I've seen the effect it has on shops: they give up on JS altogether. This is why we have dubious would-be replacements like "Dart" and "Coffeescript".
Ironically, one of the excuses given after the first review was that it was a "Beta product". Of course, that indicates they either didn't read or didn't understand the review as it called for a ground-up redesign that would be inappropriate at the Beta stage. Regardless, it's hard to apply a term like "Beta" to code with inferences that can go out of date at any moment.
As predicted, like Ext JS, Dojo, YUI and a long series of impractical, time-wasting, browser sniffing projects before them, "Sencha Touch" has become just a brand name for a never-ending series of do-overs. There's never been anything the slightest bit pragmatic about these Sisyphus imitators. What do they say about doing the same thing over and over and expecting different results? Their only pitch is "they have to do it". How silly would you have to be to keep following them up the hill?
But I digress, that's the end of the code review. Reading further would be a waste of time. At a glance, it appears that virtually every function in this thing is flawed (and in easily demonstrable ways). Certainly much of my previous review still applies.
If you don't know what you are doing, leave touch gestures alone (i.e. leave them between the browser and the user). There's no chance that including this giant pile of mistakes is going to be a value-add for your application (or Website). Ironically, it's far too large (and slow from what I've seen) for mobile use. They'll say it's "modular", but that's BS. Ask anybody who ever tried to trim down an Ext application; they always end up with virtually the entire thing (about 800K last I looked).
None of this is speculation; I've worked on projects that use Ext JS and I've talked plenty of shops out of using Sencha Touch (managed to dodge that bullet so far) by demonstrating similar solutions using 1% of the JS/CSS. I've seen the terrible advice given out in the forums and the childish reactions to criticism (and these guys want to be your latex salesmen). I've worked with many (if not all) of the host objects used by this thing and have never had to resort to UA sniffing (and I'm hardly not alone in this regard). That's the reality of Ext JS/Sencha (and most things like it).
To be fair, their salesmen think it's a must-have and inexperienced developers (typically designers with mobile app aspirations) rave about it. Of course, they've likely never read the code and have no frame of reference to compare it to anything. It's a lot like jQuery in that regard.
Also just like jQuery (and similar marketing efforts), their responses to past criticism range from complete befuddlement to playing the victim and (when all else fails) juvenile insults. That's how losers respond. In the years since, it appears they've applied themselves to learning nothing (also like the jQuery effort) and, last I checked, half of them still don't have real names (should go without saying that technical advice and code from people that use aliases should be dismissed out of hand).
Invariably designed by neophytes, these things paint themselves into corners they don't understand and then are "forced" resort to mystical incantations to try to escape. When the incantations are exposed as voodoo, the inevitable retort is that they "have to" (not mentioning the cause of course). No they didn't have to write this BS. They didn't have to misrepresent it and they don't have to continue with the charade.
I'm often accused of "not helping" these guys with my code reviews. This is not true; it's just that they don't like the answers (start over with a better grasp of the language and a competent design). I gave them the general solution to handling pointer input (i.e. mouse and touch) as well, but haven't read far enough in this new version to see whether they heeded my advice in that area. Doubtful from what I've seen, but they did get rid of that 'undefined' variable since last time (presumably in there to support touch applications that must work in NN4). :)
Considering their petulant attitude and overall ignorance (great combination), not to mention their obnoxious shills/marketers, who would want to help them? They bought their tickets; I say: let 'em crash. If you get on board with them, you are going down too. The only way you could end up not losing in this proposition is if all of your competitors willfully join you for the inevitable plunge back to earth. I suppose you can always rely on the kindness of competitors; but, if not, a faster, smaller, more accessible alternative will beat you every time.
David, a four-space indent per line will denote preserved whitespace (i.e.,
<pre>) in Markdown. That will probably help clean up your formatting.