Skip to content

Core: Deprecate jQuery.isArray #3278

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
Closed
kumarmj wants to merge 6 commits into from
Closed

Core: Deprecate jQuery.isArray #3278

kumarmj wants to merge 6 commits into from

Conversation

@kumarmj
Copy link
Contributor

@kumarmj kumarmj commented Aug 14, 2016 •
edited
Loading

fix #2961

@kumarmj kumarmj changed the title Core: Deprecate jQuery.isArray #2961 Core: Deprecate jQuery.isArray Aug 14, 2016
@kumarmj kumarmj mentioned this pull request Aug 15, 2016
Closed
@markelog
Copy link
Member

markelog commented Aug 19, 2016 •
edited
Loading

Just to note - it might take a bit of time to land, since we want to release patch version first and do this in minor. So it probably be landed at 3.2 time.

Thank you!

@mgol
Copy link
Member

mgol commented Aug 22, 2016

I don't think these are all jQuery.isArray uses in the code. Did you check all the files?

@mgol
Copy link
Member

mgol commented Aug 22, 2016

We'll also need to have some simple tests (perhaps even just one test ensuring jQuery.isArray === Array.isArray) in test/unit/deprecated.js.

@mgol mgol removed the Needs review label Aug 22, 2016
@dmethvin
Copy link
Member

dmethvin commented Aug 22, 2016

There are definitely more uses, both in src/ and in some of the subdirectories.

@dmethvin
Copy link
Member

dmethvin commented Aug 23, 2016

There are a few more still to go, like test/data/testinit.js and test/data/testrunner.js. I used this command to search: ack --ignore-dir=node_modules jQuery.isArray . If you don't have ack you might want to get it, I find it really convenient for this kind of searching.

@markelog
Copy link
Member

markelog commented Aug 23, 2016

git grep jQuery.isArray also works :)

@mgol
Copy link
Member

mgol commented Aug 23, 2016

Or "Find in Project..." in any IDE like WebStorm, Atom or Visual
Studio Code.

Michał Gołębiowski

@dmethvin
Copy link
Member

dmethvin commented Aug 23, 2016

I didn't know about git grep! The "Find in Project" option in Visual Studio has missed things in the past so I don't trust them.

@kumarmj
Copy link
Contributor Author

kumarmj commented Aug 24, 2016

thanks @dmethvin @mgol @markelog let me know if any further modifications are required. This discussions are awesome.

@dmethvin dmethvin added this to the 3.2.0 milestone Aug 25, 2016
@dmethvin
Copy link
Member

dmethvin commented Aug 25, 2016

LGTM. I've marked this for the 3.2 milestone, it won't land until after 3.1.1 ships so don't be worried if it stays unmerged for a while.

Krinkle
Krinkle reviewed Nov 25, 2016
View reviewed changes
test/unit/deprecated.js Outdated
} );

QUnit.test( "jQuery.isArray", function( assert ) {
assert.strictEqual( jQuery.isArray, Array.isArray, "jQuery.isArray raises error" );
Copy link
Member

@Krinkle Krinkle Nov 25, 2016

Choose a reason for hiding this comment

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

"raises error"?

Copy link
Contributor Author

@kumarmj kumarmj Nov 25, 2016

Choose a reason for hiding this comment

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

it's my wrong, I propose to change it as "Array.isArray covers jQuery.isArray"

Copy link
Member

@mgol mgol Nov 29, 2016

Choose a reason for hiding this comment

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

"jQuery.isArray equals jQuery.isArray" would be better IMO.

@mgol
Copy link
Member

mgol commented Nov 29, 2016

@kumarmj We'll need you to sign our new JS Foundation CLA; you'll find the link in the checks at the bottom.

@mgol
Copy link
Member

mgol commented Nov 29, 2016

LGTM. Thanks, @kumarmj!

Krinkle
Krinkle requested changes Nov 29, 2016
View reviewed changes
src/deprecated.js Outdated
this.off( types, selector || "**", fn );
}
},
isArray: Array.isArray
Copy link
Member

@Krinkle Krinkle Nov 29, 2016

Choose a reason for hiding this comment

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

This is extending jQuery.fn, not jQuery.

Copy link
Member

@mgol mgol Nov 30, 2016

Choose a reason for hiding this comment

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

Nice catch. @kumarmj I know this is now fixed but have you run the test suite before submitting the PR? It would fail the test you added in the previous version.

Krinkle
Krinkle reviewed Nov 30, 2016
View reviewed changes
src/deprecated.js
this.off( selector, "**" ) :
this.off( types, selector || "**", fn );
}

Copy link
Member

@Krinkle Krinkle Nov 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

👍 Looks good. For cleanliness, maybe remove this line which wasn't there. (Sorry)

@mgol mgol closed this in 1b9575b Nov 30, 2016
@mgol
Copy link
Member

mgol commented Nov 30, 2016

Landed. I made the following changes, please take a look @kumarmj as ideally next time you'd do it yourself:

  1. I changed the commit message so that it adheres to our format; see https://contribute.jquery.org/commits-and-pull-requests/#commit-guidelines
  2. I added a missing assert.expect( 1 ) to the jQuery.isArray test. Please run tests manually in at least one browser before submitting a PR, it would show you a failed test.
  3. I removed an unnecessary new line which @Krinkle pointed out.

Thanks for the PR!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Krinkle Krinkle Krinkle requested changes

@mgol mgol mgol left review comments

Assignees

No one assigned

Labels

None yet

Milestone

3.2.0

Development

Successfully merging this pull request may close these issues.

Deprecate jQuery.isArray

7 participants

@kumarmj @markelog @mgol @dmethvin @Krinkle @jquerybot @JSFOwner