`isArrayLike` no longer fails gracefully for null and undefined from 1.11.1 to 1.11.3 #2267

Closed
ScottLNorvell opened this Issue May 6, 2015 · 9 comments

Comments

Projects
None yet
6 participants
@ScottLNorvell

in 1.11.1, isArrayLike looks like this:

function isArraylike( obj ) {
    var length = obj.length,
        type = jQuery.type( obj );

    if ( type === "function" || jQuery.isWindow( obj ) ) {
        return false;
    }

    if ( obj.nodeType === 1 && length ) {
        return true;
    }

    return type === "array" || length === 0 ||
        typeof length === "number" && length > 0 && ( length - 1 ) in obj;
}

in 1.11.3, it looks like this:

function isArraylike( obj ) {

    // Support: iOS 8.2 (not reproducible in simulator)
    // `in` check used to prevent JIT error (gh-2145)
    // hasOwn isn't used here due to false negatives
    // regarding Nodelist length in IE
    var length = "length" in obj && obj.length,
        type = jQuery.type( obj );

    if ( type === "function" || jQuery.isWindow( obj ) ) {
        return false;
    }

    if ( obj.nodeType === 1 && length ) {
        return true;
    }

    return type === "array" || length === 0 ||
        typeof length === "number" && length > 0 && ( length - 1 ) in obj;
}

"length" in obj produces the error TypeError: Cannot use 'in' operator to search for 'length' in 6 when obj === 6. Similarly when obj is a undefined or null. This causes, specifically $.each to fail when you accidentally put in a numerical value for obj. Not sure if this is an issue, but in 1.11.1, nothing failed when obj was accidentally a number. This caused some tests to fail that we've fixed. Thought I'd mention it.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 6, 2015

Member

This is basically a dup of #2242, we don't guarantee that invalid inputs will be silently ignored.

Member

dmethvin commented May 6, 2015

This is basically a dup of #2242, we don't guarantee that invalid inputs will be silently ignored.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 6, 2015

Member

Agreed.

Member

gibson042 commented May 6, 2015

Agreed.

@gibson042 gibson042 closed this May 6, 2015

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 6, 2015

Member

@dmethvin is right, but I'm starting to wonder if it would be nice if we failed silently here. The string case was more obscure, but I would expect that changing the behavior for null and undefined will be much more problematic.

Member

timmywil commented May 6, 2015

@dmethvin is right, but I'm starting to wonder if it would be nice if we failed silently here. The string case was more obscure, but I would expect that changing the behavior for null and undefined will be much more problematic.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 6, 2015

Member

@timmywil agreed. Also, other implementations like lodash's _.each silently ignore things like null or undefined.

Member

mgol commented May 6, 2015

@timmywil agreed. Also, other implementations like lodash's _.each silently ignore things like null or undefined.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 6, 2015

Member

We use isArrayLike in three places: jQuery.each, jQuery.map, and (protected by Object) jQuery.makeArray. This is now sounding like a request to change the signatures of the former two, despite the fact that both reported instances of passing them invalid input have been corrected. And since correcting such mistakes is usually as easy as adding || [] (or if ( typeof input === "object" ) where correctness and/or performance is critical), I don't see the benefit of a change on our end. Quite the opposite, in fact—exceptions from jQuery.each( nonIterable, fn ) seem like a good thing.

Member

gibson042 commented May 6, 2015

We use isArrayLike in three places: jQuery.each, jQuery.map, and (protected by Object) jQuery.makeArray. This is now sounding like a request to change the signatures of the former two, despite the fact that both reported instances of passing them invalid input have been corrected. And since correcting such mistakes is usually as easy as adding || [] (or if ( typeof input === "object" ) where correctness and/or performance is critical), I don't see the benefit of a change on our end. Quite the opposite, in fact—exceptions from jQuery.each( nonIterable, fn ) seem like a good thing.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 6, 2015

Member

exceptions from jQuery.each( nonIterable, fn ) seem like a good thing

For the cases of numbers and strings, I agree with this, but passing variables that may be null or undefined at times to jQuery.each seems common enough that throwing is just going to be annoying to users. The question is about what is more convenient. If everyone is just going to add || [] to their code, we may as well do it in jQuery.

Member

timmywil commented May 6, 2015

exceptions from jQuery.each( nonIterable, fn ) seem like a good thing

For the cases of numbers and strings, I agree with this, but passing variables that may be null or undefined at times to jQuery.each seems common enough that throwing is just going to be annoying to users. The question is about what is more convenient. If everyone is just going to add || [] to their code, we may as well do it in jQuery.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 6, 2015

Member

The question is about what is more convenient. If everyone is just going to add || [] to their code, we may as well do it in jQuery.

That is a good point, but only if we also document that null and undefined are no-ops. I think that has been the behavior for a while so we could do that. I prefer the caller-level change to add || [] because it documents to subsequent readers that a null or undefined value was expected rather than it just happening to work. But if we change the docs at least those old calls are retroactively correct as far as their jQuery API usage goes. 😄

Member

dmethvin commented May 6, 2015

The question is about what is more convenient. If everyone is just going to add || [] to their code, we may as well do it in jQuery.

That is a good point, but only if we also document that null and undefined are no-ops. I think that has been the behavior for a while so we could do that. I prefer the caller-level change to add || [] because it documents to subsequent readers that a null or undefined value was expected rather than it just happening to work. But if we change the docs at least those old calls are retroactively correct as far as their jQuery API usage goes. 😄

@timmywil timmywil removed the Needs review label Jun 1, 2015

@timmywil timmywil added this to the 3.0.0 milestone Jun 1, 2015

@timmywil timmywil self-assigned this Jun 1, 2015

@mgol mgol changed the title from `isArrayLike` no longer fails gracefully for numbers, null, and undefined from 1.11.1 to 1.11.3 to `isArrayLike` no longer fails gracefully for null and undefined from 1.11.1 to 1.11.3 Jun 25, 2015

mr21 added a commit to mr21/jquery that referenced this issue Jun 25, 2015

@mgol mgol closed this in bf48c21 Jul 27, 2015

mgol added a commit that referenced this issue Jul 27, 2015

riichard added a commit to riichard/jquery that referenced this issue Sep 20, 2015

@uptoeleven

This comment has been minimized.

Show comment
Hide comment
@uptoeleven

uptoeleven Sep 22, 2015

So if an object that has no length attribute is passed into this function, how should this function behave?

So if an object that has no length attribute is passed into this function, how should this function behave?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Sep 22, 2015

Member

@uptoeleven This function is not exposed publicly so you need to ask about a specific public method that you use. And since those methods, like jQuery.each are usually documented so that they accept an array, an object and (but only in jQuery 3.0.0) undefined or null then if you pass in the string you're on your own.

Member

mgol commented Sep 22, 2015

@uptoeleven This function is not exposed publicly so you need to ask about a specific public method that you use. And since those methods, like jQuery.each are usually documented so that they accept an array, an object and (but only in jQuery 3.0.0) undefined or null then if you pass in the string you're on your own.

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016

@cssmagic cssmagic referenced this issue in cssmagic/ChangeLog May 18, 2016

Open

jQuery #5

@jquery jquery locked as resolved and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.