Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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
Closed

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

ScottLNorvell opened this issue May 6, 2015 · 9 comments
Assignees
@timmywil
Labels
Core
Milestone
1.12.0/2.2.0

Comments

@ScottLNorvell
Copy link

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
Copy link
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
Copy link
Member

Agreed.

@timmywil
Copy link
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
Copy link
Member

mgol commented May 6, 2015

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

@timmywil timmywil reopened this May 6, 2015
@gibson042
Copy link
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.

@timmywil
Copy link
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
Copy link
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 added this to the 3.0.0 milestone Jun 1, 2015
@timmywil timmywil self-assigned this Jun 1, 2015
@timmywil timmywil mentioned this issue Jun 1, 2015
Closed
This was referenced Jun 9, 2015
Closed
Closed
@mgol mgol changed the title isArrayLike no longer fails gracefully for numbers, null, and undefined from 1.11.1 to 1.11.3 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
@mr21
Core: .each/.map should accept an undefined value
9a77f72 
Fixes jquerygh-2267
@mgol mgol closed this as completed in bf48c21 Jul 27, 2015
mgol pushed a commit that referenced this issue Jul 27, 2015
@mr21 @mgol
Core: .each/.map should accept an undefined/null value
15f4804 
(cherry-picked from bf48c21)

Fixes gh-2267
Closes gh-2363
@mgol mgol mentioned this issue Sep 8, 2015
Closed
riichard pushed a commit to riichard/jquery that referenced this issue Sep 20, 2015
@mr21 @riichard
Core: .each/.map should accept an undefined/null value
e688a8b 
Fixes jquerygh-2267
Closes jquerygh-2363
@mgol mgol mentioned this issue Sep 21, 2015
Closed
@uptoeleven
Copy link

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

@mgol
Copy link
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.

@mgol mgol mentioned this issue Dec 7, 2015
Closed
@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016
@cssmagic cssmagic mentioned this issue May 18, 2016
Open
@gesinn-it-gea gesinn-it-gea mentioned this issue Mar 8, 2017
Closed
@lock lock bot 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.
Assignees

@timmywil timmywil

Labels
Core
Milestone
1.12.0/2.2.0
Development

Successfully merging a pull request may close this issue.

Core: .each should accept an undefined value mr21/jquery
6 participants
@dmethvin @timmywil @gibson042 @mgol @ScottLNorvell @uptoeleven