Re: SproutCore--over 20000 lines of new code!



RobG wrote:
Jorge wrote:
On Jan 9, 9:11 am, Garrett Smith <dhtmlkitc...@xxxxxxxxx> wrote:
(...)

It's so funny and ridiculous and seems sooo stupid that the same guys
that advocate turning off JS in the browser (many cljs regulars),

I don't agree that "many" cljs regulars recommend turning off javascript on a regular basis. It is certainly worth doing on sites that throw script errors or are otherwise unusable.

Unfortunately many sites are needlessly dependent on script so turning script support off is not always a useful an option. However, plugins like NoScript allow users to selectively run scripts so that they can accomplish what they want without running all scripts that a site has.

feel
themselves as an authority so as to judge somebody else's work whose
main goal/underlying idea don't even understand

The subject of this thread is from Sproutcore's home page, which also states:

"Create fast, native-style applications in any modern web browser without plugins."

It also claims to be a "JavaScript HTML5 Application Framework"

<URL: http://www.sproutcore.com/home/ >

Cljs is *the* place to discuss technical issues related to it.


(e.g., you don't know
a word of cocoa),

Utterly irrelevant. Apple's Cocoa is a "collection of frameworks, APIs, and accompanying runtimes"[1] specifically for Mac OS X developers. That is unrelated to a script library that is supposed to be suitable for web applications and presumably platforms other than Safari on Mac OS.


just by looking at a few lines in their JS source.

Those who have commented here have looked at more than "a few lines" - SproutCore is tens of thousand of lines of code, it is incredibly verbose. No one should post a review of the entire library here.


For something 20k long, I would consider a few things in assessing it:
1) What is the overall goal of the framework
2) How is is structured?

After that, I would want to look at the lowest level core and see the actual code.

I want to know that the library is using abstractions that are well-written. The core file has browser detection, global identifiers, recommendations to use either PrototypeJS or jQuery.

I see some higher level parts of the framework relying on modifications to Function.prototype, for example. I saw some place that defined certain modifications to Function.prototype, but not the modifications that were used in the datastore's renderer.

I have also noticed recent commits in GitHub with things like:-

Commit comment:
"Make it work a bit better in IE"

Where "a bit better" is a euphemism for BUGFIX.

The previous code was:-
| if (SC.none(newLayout[key])) delete style[key];

And the new code is:-
| if (SC.none(newLayout[key])) style[key] = ""; // because IE is stupid
|// and can't handle delete or debug.

What is debug?

Calling `delete` on a host object results in Errors in IE.

It is good that the author did recognize and fix the bug. But it would be hard to notice the bug without at least trying once in IE. Also, the comment should not be calling IE "stupid" for what is perfectly legal behavior. IE throwing on delete for host object is legal in ES3, through vague wording, and legal in ES5 through explicit wording via internal [[Delete]] (P, Throw).

Setting a style property to `null` IE, as another comment indicated was attempted, might also throw "Invalid Argument" errors. The program should only supply that are legal CSS values. Literal `null` might convert to domstring "null", which would either illegal CSS value (or useless things like "font family", where it would be legal).

I also see things in random places such as:-
http://github.com/sproutit/sproutcore/blob/master/frameworks/designer/css/css_style_sheet.js

My inline comments follow:

| init: function() {
| // (GS) See note below on Transcoded Function call.
| sc_super() ;
|
| var ss = this.styleSheet ;
|
| if (!ss) {
| // (GS) Extract this branch to a hidden-in-scope function.
| // create the stylesheet object the hard way (works everywhere)
| ss = this.styleSheet = document.createElement('style') ;
| ss.type = 'text/css' ;
| var head = document.getElementsByTagName('head')[0] ;
|
| // (GS) What does this fix for Opera?
| if (!head) head = document.documentElement ; // fix for Opera
| head.appendChild(ss) ;
| }
|
| // (GS) Move this "get or create" Factory logic elsewhere. It does not
| // (GS) belong in init().
| // cache this object for later
| var ssObjects = this.constructor.styleSheets ;
| if (!ssObjects) ssObjects = this.constructor.styleSheets = {} ;
| ssObjects[SC.guidFor(ss)] ;
|
| // (GS) This is either undefined or an IE `rules` collection.
| // create rules array
|
| var rules = ss.rules || SC.EMPTY_ARRAY ;
| var array = SC.SparseArray.create(rules.length) ;
|
| // (GS) Avoid leaking implementation details to the system.
| array.delegate = this ;
| this.rules = array ;
|
| return this ;
| },

The transcoded function call, mentioned the first review comment, is apparently changed to:- arguments.callee.base.apply(this, arguments);

That statement, used in the `init` method below, means that `arguments.callee` *is* the `init` method. init.base is not defined in `SC.CSSStyleSheet`. It seems as though it is defined as an extension to `Function.prototype`, though at this point, it might be transcoded to something else.

Transcoding sc_super() creates a confusing coupling with the build tool. That confusion and complexity is not justified by the syntactic sugar it provides.

To the author: Replace that statement with real code and stop trying to be clever.

We also see that in the actual function, there is a caching mechanism. This "get or create" can be useful, but I don't think it belongs there in `init`.

There is also the problem in that this method will only work in a browser that has an IE `rules` collection. That includes IE and Safari, and I don't know what else. Instead, the standard `cssRules` property should be used with `rules` as a fallback.

I did not look at SC.SparseArray.create. I assume it is about on quality with everything else seen so far.
--
Garrett
comp.lang.javascript FAQ: http://jibbering.com/faq/
.