You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Paul Lindner <li...@inuus.com> on 2012/01/03 09:45:27 UTC
Re: Review Request: Changing equality checks against undefined to typeof
checks against 'undefined'
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3332/#review4174
-----------------------------------------------------------
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;
};
- Paul
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
>
>
Re: Review Request: Changing equality checks against undefined to typeof
checks against 'undefined'
Posted by Dan Dumont <dd...@us.ibm.com>.
> 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;
> > };
> >
Paul, is google's deployment running with advanced optimizations turned on?
- 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
>
>
Re: Review Request: Changing equality checks against undefined to typeof
checks against 'undefined'
Posted by Ryan Baxter <rb...@gmail.com>.
> 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.
maybe add it to core.util? Or possibly have its own feature which you add as a dependency to core...
- Ryan
-----------------------------------------------------------
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
>
>
Re: Review Request: Changing equality checks against undefined to typeof
checks against 'undefined'
Posted by Dan Dumont <dd...@us.ibm.com>.
> 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
>
>
Re: Review Request: Changing equality checks against undefined to typeof
checks against 'undefined'
Posted by Stanton Sievers <si...@gmail.com>.
> 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?
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.
- Stanton
-----------------------------------------------------------
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
>
>