Core: .each should accept an `undefined` value by mr21 · Pull Request #2363 · jquery/jquery

mr21 · 2015-05-31T03:31:06Z

Hi :)

I found in JS we have this:

var n = 0;
n.length === undefined;
false.length === undefined;
true.length === undefined;
NaN.length === undefined;

// There is an exception here:
undefined.length; // crash

And with the actual jQuery, sometime we have to do this:

if (obj)
    $.each(obj, function() {
        // ...
    });

If we reorder few instructions in the $.each function we can remove the necessity of the if above.

Do you agree?

EDIT (by @mzgol): Fixes gh-2267

timmywil · 2015-06-01T14:15:37Z

Thank you for opening a PR. We've discussed this issue here. It also affects .map. Would you be up for opening a PR for that too? I think .each and .map do need adjustments, since they check length before the call to isArraylike, but if it would save anything to make changes there as well, feel free.

mgol · 2015-06-01T15:16:01Z

Such a change requires a test, though.

mr21 · 2015-06-01T17:09:51Z

Oh right, cool, I will do this (the other PR and the test), this evening.
Thanks :)

mr21 · 2015-06-02T01:03:52Z

I added a test, but this test fail actually, because modifying $.each is not enough... I have to reorder the isArraylike function, I made this change here #2368.
Maybe you can merge the .map PR before this one.

markelog · 2015-06-02T03:18:30Z

If this PR should fix #2267 you probably should add tests for all those values

mgol · 2015-06-02T12:40:07Z

@mr21 a PR should close an issue if it changes behavior so perhaps you could consolidate the changes in one PR that would fix #2267?

markelog · 2015-06-02T12:57:32Z

One PR is preferable, but we could do two atomic commits, one would ref, other would close

timmywil · 2015-06-02T15:07:35Z

Sorry, I should have suggested one PR.
On Tue, Jun 2, 2015 at 08:57 Oleg Gaidarenko notifications@github.com
wrote:

One PR is preferable, but we could do two atomic commit, one would ref,
other would close

—
Reply to this email directly or view it on GitHub
#2363 (comment).

mr21 · 2015-06-02T23:56:06Z

So, I closed the other PR about .map.
I restart this PR too, can you review this first (atomic) commit about the fix of isArraylike?

mr21 · 2015-06-02T23:59:57Z

Here the code return false for: null, undefined, "number", NaN, "function", "window" only.

This is very hard to read. Please assign type in its declaration, and remove the type === "number" check completely. Also, it may be smaller to assign length in its declaration as well (e.g., length = obj != null && "length" in obj && obj.length).

gibson042 · 2015-06-03T13:12:54Z

This PR needs a reference to #2267 and unit tests for passing null/undefined to jQuery.each and jQuery.map (and corresponding changes to those methods so they don't throw on accessing .length).

mr21 · 2015-06-03T14:29:46Z

@gibson042, yeah I know I did it yesterday but markelog spoke about two atomic commits.
Have I to make only two commits here or it's you after who will reorganize all the code of the PR before the push on master?
It's the reason why I didn't add my unit test today, because I wanted to be sure about isArraylike.

gibson042 · 2015-06-03T14:43:38Z

Just add relevant commits for both jQuery.each and jQuery.map to this branch, which will close an updated #2267 when it lands. isArraylike will only need changes as necessary to support the newly-valid null and undefined input for those public methods.

mr21 · 2015-06-07T20:38:48Z

@gibson042 all is done :)

mgol · 2015-06-25T01:01:40Z

@mr21 Could you squash your commits and add Fixes gh-2267 to the commit message? I modified its title so that it doesn't mention numbers which are not officially handled. See d471842 for an example commit message.

Fixes jquerygh-2267

mr21 · 2015-06-25T04:21:27Z

@mzgol, right, it's done :)

mgol · 2015-06-25T14:08:05Z

You mention Function here but the test is only about undefined/null.

Oh it's because I am testing :

$.each( null || undefined, function() { });

Like the title of the precedent test ( jQuery.each(Object,Function) )

Ah, it's a comma, not a slash. OK, sorry. :)

This landed and nobody noticed that it should have used expect( 1 ) instead of the deprecated argument =/

Oh the humanity! How can we possible recover from this horrible-horrible mistake?!
...
Sorry, couldn't resist.

We will fix all of that occurrences at some point.

mgol · 2015-07-27T17:48:24Z

LGTM now.

(cherry-picked from bf48c21) Fixes gh-2267 Closes gh-2363

mgol · 2015-07-27T18:09:57Z

Landed, thanks!

Fixes jquerygh-2267 Closes jquerygh-2363

jquerybot added the CLA: Valid label May 31, 2015

mr21 mentioned this pull request Jun 1, 2015

Core: .map should accept an undefined value #2368

Closed

markelog reviewed Jun 2, 2015
View reviewed changes

Comment thread test/unit/core.js Outdated

Copy link
Copy Markdown

Member

markelog Jun 2, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this PR should fix #2267 you probably should add tests for all those values

mr21 force-pushed the each-undefined branch from 4f9cf25 to cc9c714 Compare June 2, 2015 23:54

mr21 reviewed Jun 2, 2015
View reviewed changes

mr21 force-pushed the each-undefined branch from cc9c714 to 8d1db1d Compare June 7, 2015 20:35

mr21 mentioned this pull request Jun 15, 2015

$.each fails with undefined #2395

Closed

mgol added the Needs review label Jun 25, 2015

Core: .each/.map should accept an undefined value

9a77f72

Fixes jquerygh-2267

mr21 force-pushed the each-undefined branch from 8d1db1d to 9a77f72 Compare June 25, 2015 04:18

mgol reviewed Jun 25, 2015
View reviewed changes

timmywil removed the Needs review label Jul 6, 2015

mgol self-assigned this Jul 8, 2015

Merge branch 'master' into each-undefined

2d1766f

mr21 mentioned this pull request Jul 21, 2015

jQuery 2.1.4 Bug in my web page #2383

Closed

Core: makeArray coding style rollback

1ca677d

mgol closed this in bf48c21 Jul 27, 2015

mgol pushed a commit that referenced this pull request Jul 27, 2015

Core: .each/.map should accept an undefined/null value

15f4804

(cherry-picked from bf48c21) Fixes gh-2267 Closes gh-2363

riichard pushed a commit to riichard/jquery that referenced this pull request Sep 20, 2015

Core: .each/.map should accept an undefined/null value

e688a8b

Fixes jquerygh-2267 Closes jquerygh-2363

lock Bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Conversation

mr21 commented May 31, 2015

Uh oh!

timmywil commented Jun 1, 2015

Uh oh!

mgol commented Jun 1, 2015

Uh oh!

mr21 commented Jun 1, 2015

Uh oh!

mr21 commented Jun 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgol commented Jun 2, 2015

Uh oh!

markelog commented Jun 2, 2015

Uh oh!

timmywil commented Jun 2, 2015

Uh oh!

mr21 commented Jun 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibson042 commented Jun 3, 2015

Uh oh!

mr21 commented Jun 3, 2015

Uh oh!

gibson042 commented Jun 3, 2015

Uh oh!

mr21 commented Jun 7, 2015

Uh oh!

mgol commented Jun 25, 2015

Uh oh!

mr21 commented Jun 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgol commented Jul 27, 2015

Uh oh!

mgol commented Jul 27, 2015

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants