code review for controlJS

[Oleg Slobodskoi]

Hi Steve,

I just took a look at your code and want to give you my feedback:

1. you are executing the code using "onload" event, which is fired
when complete website is downloaded including images etc. , "domready"
should work for your script too and execute much earlier.

2. I suppose you have already heared about closures in js which enable
you to have a bit privacy? :) The most functions in your code could be
defined as private and enable minifiers to rewrite them and make the
code smaller :)

3. CJS.eval - this will work for IE correctly, but window.eval gives
access to the current scope for the evaled code. This is ok if you
write your whole code in global scope, but if not - it could have side
effects, so take a look at "globalEval" method of jQuery.

4. document.write override is not possible to do for all usecases.
Example:
document.write("<script src="this/script/makes/even/more/
document.write/with/external/scripts"></script>")

Actually you have to download the code, look if there some more
document.write, look if there a script tags with external ressources,
download them and look for scripts again, and then have fun executing
all this without to brake it :)

This stuff is awful but it really exists in widgets code.

5. There is also "document.writeln" :)

6. If there are a lot of scripts and you are loading them async - it
is possible that you reach max connections browser limit and instead
to enable non-blockin loading of the page - you will block it because
images on the page will have to wait for scripts.

7. You didn't write any unit tests :) - how do you want to ensure your
code works in all browsers now and after some changes ....

regards,
Oleg

@oleg008
github.com/kof

[Steve Souders]

Hi, Oleg.

Thanks for taking the time to do this. I'll admit I was more focused on the concepts than the robustness of the code, but it'll be great to fix it up.

1. I don't think "domready" is implemented in most browsers.

2. Technically, minifiers remove whitespace and mungers (or obfuscators) change variable names. I like minifiers. I don't use mungers - they introduce too many bugs to justify the small size savings. But this is really a programming style. I find it more familiar to work this way, esp. early on when I'm not sure how the code is going to shake out. But I agree - in a few months it might be appropriate to use closures.

3. Do you have a test case that fails? That would help a lot!

4. Yes, as I mentioned several times in the blog post ControlJS's solution won't handle these complex edge cases. And I'm not saying they don't occur - many ads will do multiple levels of document.write (unfortunately).

5. Oh - yea. I opened a bug for document.writeln.

6. This is unlikely to be an issue - certainly less of an issue than it would be in the page without ControlJS. ControlJS doesn't start downloading scripts until control.js is loaded. While control.js is being loaded (and parsed) the page itself continues to be parsed and the browser will kick off requests for those images in the page. In fact, if you look at the async.php example you'll see that even though the images are last in the page they get downloaded before main.js and page.js just for this reason. It's possible in IE6&7 (that don't support parallel script loading) you could have a situation where the script downloads gobble up more connections that normal, but I would bet not.

7. Let's add them!

-Steve

[Oleg Slobodskoi]

2010/12/17 Steve Souders <st...@souders.org>:

> Hi, Oleg.
>
> Thanks for taking the time to do this. I'll admit I was more focused on the
> concepts than the robustness of the code, but it'll be great to fix it up.
>
> 1. I don't think "domready" is implemented in most browsers.

No, but there are some working workarounds for it. See jquery.

>
> 2. Technically, minifiers remove whitespace and mungers (or obfuscators)
> change variable names. I like minifiers. I don't use mungers - they
> introduce too many bugs to justify the small size savings. But this is
> really a programming style. I find it more familiar to work this way, esp.
> early on when I'm not sure how the code is going to shake out. But I agree -
> in a few months it might be appropriate to use closures.
>
> 3. Do you have a test case that fails? That would help a lot!

// Eval a string of JavaScript in the proper context.
var test = function(code) {
//CJS.dprint("evaling: " + code.substring(0, 64));

if (window.execScript) {
window.execScript(code);
}
else {
var fn = function() {
window.eval.call(window, code);
};
fn();
}

//CJS.dprint("eval exit");
};
test("alert(fn+'');")



this is your eval method, and in the alert you see the function which
was introduced in the scope where you doing eval. so every evaled code
is able to overwrite it ;)

>
> 4. Yes, as I mentioned several times in the blog post ControlJS's solution
> won't handle these complex edge cases. And I'm not saying they don't occur -
> many ads will do multiple levels of document.write (unfortunately).

Yes but the problem is, user don't understand this difference and they
never know if an ADD has "multiple levels of document.write" or not -
the result for them will be broken website.

>
> 5. Oh - yea. I opened a bug for document.writeln.
>
> 6. This is unlikely to be an issue - certainly less of an issue than it
> would be in the page without ControlJS. ControlJS doesn't start downloading
> scripts until control.js is loaded. While control.js is being loaded (and
> parsed) the page itself continues to be parsed and the browser will kick off
> requests for those images in the page. In fact, if you look at the async.php
> example you'll see that even though the images are last in the page they get
> downloaded before main.js and page.js just for this reason. It's possible in
> IE6&7 (that don't support parallel script loading) you could have a
> situation where the script downloads gobble up more connections that normal,
> but I would bet not.

Ok, in this case it might work even for a lot of scripts. However I
was thinking to minify controljs and to put on top of "head" inline,
so no wasted time for downloading it. In this case we need smallest
possible size of it, so - use closure.

[Will Alexander]

>
> 4. document.write override is not possible to do for all usecases.
> Example:
> document.write("<script src="this/script/makes/even/more/
> document.write/with/external/scripts"></script>")
>
> Actually you have to download the code, look if there some more
> document.write, look if there a script tags with external ressources,
> download them and look for scripts again, and then have fun executing
> all this without to brake it :)
>
> This stuff is awful but it really exists in widgets code.

Trust me, to fully support document.write() ControlJS would have to
triple in size!

If you're interested in a solution check out GhostWriter as Steve
suggested in his post. It provides near perfect pre-load emulation
and handles this kind of nesting. It's done 100% in Javascript so you
start testing immediately.

http://digital-fulcrum.com/solutions/ghostwriter-complete-control/

[Oleg Slobodskoi]

here is a nice article about code evaluation in global scope (from
papa kangax :) )

http://perfectionkills.com/global-eval-what-are-the-options/