> On 2012-01-03 08:45:27, Paul Lindner wrote:
> > I'm okay with the patch as-is, but would prefer something like this which
> > is a modified version of the closure library goog.isDef
> >
> >
> > goog.isDef = function(val) {
> > var undefined;
> > return val !== undefined;
> > };
> >
>
> Dan Dumont wrote:
> Paul, is google's deployment running with advanced optimizations turned
> on?
>
> Stanton Sievers wrote:
> Paul, where would such a definition live such that it is accessible
> across all of the JavaScript files? Any idea how we could hook that up? I'd
> be up for trying that... although, if we were going to introduce goog.isDef I
> would like to replace all typeof instances with that, which is going to be a
> much bigger change.
>
> Ryan Baxter wrote:
> maybe add it to core.util? Or possibly have its own feature which you
> add as a dependency to core...
This is the concern I have:
Even being generous with the name (for a public API I would rather see a more
complete name: isDefined) and location of the new util
(shindig.util.isDef(foo))
That's 23 characters long to check if a variable is undefined.
The alternative is the regular undefined check typeof(foo)!='undefined' (24
characters)
The shindig exported namespaces are rather long, and utility is supposed to be
easy to remember and more useful than plain js usage.
I'm not sure the added function call and 1 character savings is worth the
addition of the API.
Paul, is there any benefit to the isDef checking method over the typeof
checking? Or is the only benefit the rather small length namespace offered by
goog.isDef (which clashes with how utility is organized in shindig)?
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3332/#review4174
-----------------------------------------------------------
On 2011-12-29 20:05:42, Stanton Sievers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3332/
> -----------------------------------------------------------
>
> (Updated 2011-12-29 20:05:42)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> The purpose of this patch is to remove equality checking against the
> "undefined" keyword, i.e., "foo != undefined". I've replaced such instances
> with "typeof foo != 'undefined'" instead. This eliminates possible errors
> that can arise if the "undefined" keyword is reassigned.
>
> A little background regarding the motivation for this patch.... As of
> ECMAScript 3 and before ECMAScript 5, undefined is a keyword but is not
> restricted, thus, it's value can be re-assigned. This is generally caused by
> a programming error, such as assigning values to object attributes without
> first checking that the object attribute exists - making them undefined.
> When these types of errors occur they are difficult to track down and debug.
> This patch is one way to help reduce possible errors caused by undefined
> being re-assigned.
>
> To find instances to replace, I searched for this regular expression in the
> code base using Eclipse search within *.js files: =[ ]*undefined[ ]*
>
> I ignored certain files, such as external libs (CodeMirror, etc) and most
> tests.
>
>
> Diffs
> -----
>
>
> http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/cconviews.js
> 1225624
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js
> 1225624
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js
> 1225624
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.json/json-flatten.js
> 1225624
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/i18n/datetimeparse.js
> 1225624
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/open-views/viewenhancements-container.js
> 1225624
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/shindig.container/shindig-container.js
> 1225624
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/shindig.uri/uri.js
> 1225624
>
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/osapi/batchtest.js
> 1225624
>
> Diff: https://reviews.apache.org/r/3332/diff
>
>
> Testing
> -------
>
> Built everything. Ensured that the common container sample gadgets still
> render.
>
>
> Thanks,
>
> Stanton
>
>
