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.

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

Avoid use of Object.keys() [corrected] #1472

Closed
wants to merge 1 commit into from

Conversation

hiroshi-maybe
Copy link

Avoid use of Object.create() to keep code consistency for iteration of object properties.

@staabm
Copy link
Contributor

staabm commented Dec 31, 2013

Did you perf test the change? Usually You also need to open a bugtracker ticket

@timmywil
Copy link
Member

An associated ticket on the bug tracker is necessary.

For more information, see the contributing guidelines.

@timmywil
Copy link
Member

Also, I think you meant Object.keys.

@markelog
Copy link
Member

/cc @rwaldron

@hiroshi-maybe
Copy link
Author

Thanks for comments! I opened a ticket in bugtracker. http://bugs.jquery.com/ticket/14659
I will recreate a patch to modify wrong commit log if change is accepted.

@staabm Thanks for confirmation. No errors are found in Test Suite. Also I verified that event listeners were removed in my simple test case.

@rwaldron
Copy link
Member

The change works fine, but I don't like the rationale given in the ticket.

@K-ori considering my response here: http://bugs.jquery.com/ticket/14659#comment:1, can you provide evidence of a performance improvement?

@hiroshi-maybe
Copy link
Author

[Performance]
I created jsperf benchmark. for-in was faster if hasOwnProperty() is not called (Chrome 31.0/Safari 6.0.5 on OS X 10.8.5).
http://jsperf.com/enumerate-properties

[File size]
for-in is smaller than Object.create(). I quote snippet of build script.

master branch

raw gz minify
245170 72599 dist/jquery.js
83790 29329 dist/jquery.min.js

patched branch

raw gz minify
245096 72570 dist/jquery.js
83743 29307 dist/jquery.min.js

Compared to master @ 537e9ce

raw gz minify
-209 -53 dist/jquery.js
-111 -29 dist/jquery.min.js

@dmethvin
Copy link
Member

dmethvin commented Feb 7, 2014

It looks like we're just backing out the change from c1b8edf, which is fine with me if it saves bytes.

@dmethvin dmethvin added this to the 1.11.1/2.1.1 milestone Feb 7, 2014
@dmethvin
Copy link
Member

@mzgol addressed this with a slightly different patch in a96d5be . Thanks for pointint this out @K-ori !

@dmethvin dmethvin closed this Mar 14, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants