Re: scary Javascript
On Thu, Dec 11, 2008 at 9:39 PM, Paul Lindner plind...@hi5.com wrote:
Thanks for parsing the jslint output.
FWIW this is the very same JS code that is running in most opensocial
production instances today. To say it's not production ready just not
true. The truth is that this code fails to pass an arbitrary set of
guidelines. Try running PMD or findbugs on the Java code and you'll get a
simliar list of scary errors that need to be fixed right now!
Also, jslint is just stupid in some sections. For example:
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/core/legacy.js:190:63:'escape'
is undefined.
return window.encodeURIComponent ? encodeURIComponent(str) : escape(str);
This is code that attempts to use encodeURIComponent on browsers that
support it and falls back to escape() for ancient browsers. This is
probably overkill since I doubt that OpenSocial performs very well on
Netscape Navigator or IE4 :)
In any case, I'll see about cleaning up some of these if it pushes things
along.
For the future it might be nice to enable the jslint maven plugin for the
release target. That combined with standard criteria might serve us well
for future revs.
We do have it enabled, but the default configuration is worthless, with
warnings like only one var statement should be used per scope polluting
any useful output.
On Dec 11, 2008, at 8:41 PM, Henning P. Schmiedehausen wrote:
As I mentioned before, I ran jslint in a very light setting on the
features javascript code. After applying the changes in SHINDIG-776 to
the tree, this leaves me with the following messages:
a) eval is evil:
While jslint flags these, I don't see a simple way to change
this. IMHO in these places, eval is probably the right answer. So no
reason to changes this unless someone comes up with a good solution to
this.
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/
core.io/io.js:173:15:eval is evil.
var data = eval(( + txt + ));
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/core/auth.js:154:16:eval
is evil.
trusted = eval(( + config.trustedJson + ));
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/core/json.js:159:15:eval
is evil.
return eval('(' + text + ')');
^
b) undefined methods:
I have no ideas where those methods are and what they do. Typos? Remnants?
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/core/legacy.js:190:63:'escape'
is undefined.
return window.encodeURIComponent ? encodeURIComponent(str) : escape(str);
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/core/legacy.js:199:63:'unescape'
is undefined.
return window.decodeURIComponent ? decodeURIComponent(str) :
unescape(str);
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/core/util.js:119:67:'unescape'
is undefined.
var unesc = window.decodeURIComponent ? decodeURIComponent : unescape;
^
c) unclear breaks:
Some of these seem to be intentional fallthroughs but this is sloppy
code at best. Either use if elseif else or document clearly what the
code does.
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/flash/flash.js:81:58:Expected
a 'break' statement before 'case'.
swfContainer = document.getElementById(swfContainer);
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/flash/flash.js:85:6:Expected
a 'break' statement before 'default'.
}
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/flash/flash.js:92:21:Expected
a 'break' statement before 'case'.
opt_params = {};
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/views/views.js:155:22:Expected
a 'break' statement before 'case'.
flag = 1;
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/views/views.js:181:22:Expected
a 'break' statement before 'case'.
flag = 1;
^
d) scary for loops
The explanation on the jslint homepage is sufficient enough for me
that I don't like these:
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/flash/flash.js:151:8:The
body of a for in should be wrapped in an if statement to filter unwanted
properties from the prototype.
for (var attrProp in attr) {
^
/home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/opensocial-base/jsonactivity.js:75:2:The
body of a for in should be wrapped in
Re: scary Javascript
On Fri, Dec 12, 2008 at 2:19 AM, Paul Lindner plind...@hi5.com wrote: On Dec 12, 2008, at 2:14 AM, Kevin Brown wrote: On Thu, Dec 11, 2008 at 9:39 PM, Paul Lindner plind...@hi5.com wrote: Thanks for parsing the jslint output. FWIW this is the very same JS code that is running in most opensocial production instances today. To say it's not production ready just not true. The truth is that this code fails to pass an arbitrary set of guidelines. Try running PMD or findbugs on the Java code and you'll get a simliar list of scary errors that need to be fixed right now! Also, jslint is just stupid in some sections. For example: /home/henning/ning/shindig/git/shindig/features/src/main/javascript/features/core/legacy.js:190:63:'escape' is undefined. return window.encodeURIComponent ? encodeURIComponent(str) : escape(str); This is code that attempts to use encodeURIComponent on browsers that support it and falls back to escape() for ancient browsers. This is probably overkill since I doubt that OpenSocial performs very well on Netscape Navigator or IE4 :) In any case, I'll see about cleaning up some of these if it pushes things along. For the future it might be nice to enable the jslint maven plugin for the release target. That combined with standard criteria might serve us well for future revs. We do have it enabled, but the default configuration is worthless, with warnings like only one var statement should be used per scope polluting any useful output. That's actually the yui-compressor output... I'm guessing the jslint could be configured in a more fine-grained manner. d'oh, yeah, you're right.
Re: scary Javascript
On Dec 12, 2008, at 2:14 AM, Kevin Brown wrote: On Thu, Dec 11, 2008 at 9:39 PM, Paul Lindner plind...@hi5.com wrote: Thanks for parsing the jslint output. FWIW this is the very same JS code that is running in most opensocial production instances today. To say it's not production ready just not true. The truth is that this code fails to pass an arbitrary set of guidelines. Try running PMD or findbugs on the Java code and you'll get a simliar list of scary errors that need to be fixed right now! Also, jslint is just stupid in some sections. For example: /home/henning/ning/shindig/git/shindig/features/src/main/ javascript/features/core/legacy.js:190:63:'escape' is undefined. return window.encodeURIComponent ? encodeURIComponent(str) : escape(str); This is code that attempts to use encodeURIComponent on browsers that support it and falls back to escape() for ancient browsers. This is probably overkill since I doubt that OpenSocial performs very well on Netscape Navigator or IE4 :) In any case, I'll see about cleaning up some of these if it pushes things along. For the future it might be nice to enable the jslint maven plugin for the release target. That combined with standard criteria might serve us well for future revs. We do have it enabled, but the default configuration is worthless, with warnings like only one var statement should be used per scope polluting any useful output. That's actually the yui-compressor output... I'm guessing the jslint could be configured in a more fine-grained manner.
