incubator-callback-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Filip Maj <...@adobe.com>
Subject Re: `instanceof` considered harmful in cordova-js
Date Fri, 27 Apr 2012 18:57:23 GMT
Pretty much for all cases we are checking native types, so the toString()
approach *should* work here.

Here's the grep output from cordova-js, and I'll provide context for each
little bit below the grep output. Five general cases. All but one
instanceof check against a native type (and that one we can change away
from testing against one of our defined types).

----------

lib/blackberry/plugin/blackberry/Contact.js:104:        if
(contact.birthday instanceof Date) {
lib/common/plugin/Contact.js:28: if (!value instanceof Date){
lib/common/plugin/Contact.js:35: if (value instanceof Date){


Either assigns as a date property or parses the (assumed) string into a
Date, to eventually call .valueOf() to get the timestamp number.

lib/blackberry/plugin/blackberry/Contact.js:113:    if (contact.emails &&
contact.emails instanceof Array) {
lib/blackberry/plugin/blackberry/Contact.js:141:    if
(contact.phoneNumbers && contact.phoneNumbers instanceof Array) {
lib/blackberry/plugin/blackberry/Contact.js:189:    if (contact.addresses
&& contact.addresses instanceof Array) {
lib/blackberry/plugin/blackberry/Contact.js:202:            if (!address
|| address instanceof ContactAddress === false) {
lib/blackberry/plugin/blackberry/Contact.js:217:    if (contact.urls &&
contact.urls instanceof Array) {
lib/blackberry/plugin/blackberry/Contact.js:239:    if
(contact.organizations && contact.organizations instanceof Array) {
lib/blackberry/plugin/blackberry/Contact.js:261:    if (contact.categories
&& contact.categories instanceof Array) {
lib/blackberry/plugin/blackberry/Contact.js:277:    if (contact.photos &&
contact.photos instanceof Array) {
lib/blackberry/plugin/blackberry/contacts.js:18:        if (!fields ||
!(fields instanceof Array) || fields.length === 0) {
lib/blackberry/plugin/blackberry/ContactUtils.js:156: if (fields && fields
instanceof Array) {
lib/blackberry/plugin/blackberry/ContactUtils.js:216: if (!fields ||
!(fields instanceof Array) || fields.length === 0) {
lib/common/plugin/contacts.js:22: if (!fields || (fields instanceof Array
&& fields.length === 0)) {



All of the Array checks here serve the purpose of making sure the object
has a .length property as we iterate over these.

lib/common/channel.js:137:    if (typeof c == "object" && f instanceof
Function) { func = utils.close(c, f); }
lib/common/channel.js:163:        if (typeof c == "object" && f instanceof
Function) { f = utils.close(c, f); }
lib/common/channel.js:178:    if (g instanceof Function) { g =
g.observer_guid; }
lib/common/channel.js:194:            if (handler instanceof Function) {
lib/cordova.js:35: if (channel.onResume.fired && handler instanceof
Function) {


Making sure we can .apply() the function.

lib/common/utils.js:24:        if(obj instanceof Array){
lib/common/utils.js:32:        if (obj instanceof Function) {
lib/common/utils.js:36:        if(!(obj instanceof Object)){
lib/common/utils.js:40:        if(obj instanceof Date){

These ones are used in the .clone method. Array is iterated over and
cloned, the rest are returned.

lib/ios/exec.js:72:        } else if (typeof(arg) == 'object' && !(arg
instanceof Array)) {

The iOS exec case here checks if an argument in the exec() call is an
object (and not an array), and assigns it so that the object can be parsed
with named arguments on the native side.



On 4/27/12 11:38 AM, "Brian LeRoux" <b@brian.io> wrote:

>Would like to know the exact offending piece of code too. I'd question
>the need of even having that check.... its good to be defensive but
>this code would just silently return instead of failing noisily
>anyhow.
>
>On Fri, Apr 27, 2012 at 11:29 AM, Jesse <purplecabbage@gmail.com> wrote:
>> Channel.prototype.subscribe = function(f, c, g) {
>>    // need a function to call
>>    if (f === null || f === undefined) { return; }
>>
>>    var func = f;
>>    if (typeof c == "object" && f instanceof Function) { func =
>> utils.close(c, f); }
>>
>>
>> I assume we are talking about the above in our code.
>>
>>
>> Can we get away with simply doing this? :
>>
>> Channel.prototype.subscribe = function(f, c, g) {
>>    // need a function to call
>>    if ( !f || !f.apply ) { return; }
>>
>>    var func = f;
>>    if (typeof c == "object") { func = utils.close(c, f); }
>>
>>
>>
>>
>> On Fri, Apr 27, 2012 at 11:22 AM, Michael Brooks
>> <michael@michaelbrooks.ca>wrote:
>>
>>> I believe the latest jQuery source has dropped support for a lot of
>>>legacy
>>> browsers. Regardless, even release 1.6.1 uses the implementation.
>>>
>>> On Fri, Apr 27, 2012 at 11:17 AM, Filip Maj <fil@adobe.com> wrote:
>>>
>>> > For reference: https://issues.apache.org/jira/browse/CB-588
>>> >
>>> > Essentially: using instanceof across windows (basically: iframes) is
>>>bad.
>>> > Daniel from the issue thread above posted a link [1] explaining the
>>> > underlying issue in a bit more detail.
>>> >
>>> > In this specific example, GWT apps use an iframe to create
>>>initialization
>>> > callbacks and whatnot. Attaching these callbacks to deviceready (or
>>>other
>>> > cordova events) in the parent window doesn't work because instanceof
>>> > doesn't work across frames.
>>> >
>>> > The link [1] offers a workaround: calling toString() on whatever you
>>>want
>>> > to check the instance of and comparing it against "[object
>>> > myPrototypeName]". My initial thought is, that seems weak, but
>>>taking a
>>> > look at jQuery source (see [2] and [3] together), they use this same
>>> > approach. That tells me it's pretty battle hardened.
>>> >
>>> > I'm leaning towards employing the toString() approach instead of a
>>> > combination of typeof and duck-typing to fix this issue.
>>> >
>>> > I wanted to run it by the list to make sure this sounds alright and
>>>see
>>> if
>>> > anyone has any problems with this.
>>> >
>>> > Cheers,
>>> > Fil
>>> >
>>> > [1]
>>> >
>>> 
>>>http://perfectionkills.com/instanceof-considered-harmful-or-how-to-write
>>>-a-
>>> > robust-isarray/
>>> > [2] 
>>>https://github.com/jquery/jquery/blob/master/src/core.js#L471-L491
>>> > [3] 
>>>https://github.com/jquery/jquery/blob/master/src/core.js#L900-L902
>>> >
>>> >
>>>


Mime
View raw message