Skip to content

[1.8.2] Fix #12229: size/consistency improvements #887

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

[1.8.2] Fix #12229: size/consistency improvements #887

gibson042 wants to merge 6 commits into from

Conversation

@gibson042
Copy link
Member

@gibson042 gibson042 commented Aug 9, 2012

Slightly smaller size, but still smaller.

Sizes - compared to master @ 1d8bf0a

    258442       (-52)  dist/jquery.js                                         
     92532       (-28)  dist/jquery.min.js                                     
     33084       (-21)  dist/jquery.min.js.gz

@gibson042
Copy link
Member Author

gibson042 commented Aug 20, 2012

Good points as usual, @dmethvin.

@gibson042
Copy link
Member Author

gibson042 commented Sep 18, 2012

@dmethvin Can we get this in 1.8.2? I'd like a nice base for the hackathon.

@dmethvin
Copy link
Member

dmethvin commented Sep 18, 2012

Was just working on this, yeah.

rwaldron
rwaldron reviewed Sep 18, 2012
View reviewed changes
src/data.js
Copy link
Member

@rwaldron rwaldron Sep 18, 2012

Choose a reason for hiding this comment

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

Being explicit is faster then coercion, I suspect that extend is a perf sensitive operation...

Copy link
Member

@dmethvin dmethvin Sep 18, 2012

Choose a reason for hiding this comment

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

I don't think it's in a path that hot; this particular code only runs once to pull in the data attrs.

Copy link
Member

@rwaldron rwaldron Sep 18, 2012

Choose a reason for hiding this comment

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

it's a nested loop...?

Copy link
Member

@jaubourg jaubourg Sep 18, 2012

Choose a reason for hiding this comment

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

I love hot paths.

Copy link
Member

@rwaldron rwaldron Sep 18, 2012

Choose a reason for hiding this comment

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

I love hot pants

Copy link
Member

@dmethvin dmethvin Sep 18, 2012

Choose a reason for hiding this comment

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

Nested? https://github.com/gibson042/jquery/blob/f7067566d1bda266e8bc9bc7c51d8755d068fbd4/src/data.js#L230

Copy link
Member

@rwaldron rwaldron Sep 18, 2012

Choose a reason for hiding this comment

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

I'd like to call a friend on this one... my eyes tricked me into thinking this was deeper then it is.

Day 1 of TC39 + trying to look at PRs. Whoops!

Copy link
Member

@dmethvin dmethvin Sep 18, 2012

Choose a reason for hiding this comment

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

NP, just wanted to be sure I didn't miss something.

@dmethvin dmethvin closed this in 15b5dbf Sep 18, 2012
@dmethvin
Copy link
Member

dmethvin commented Sep 18, 2012

I liked all the changes, just spent some time trying to convince myself they were all truly innocuous since we're closed to 1.8.2.

@rwaldron rwaldron mentioned this pull request Oct 21, 2012
Closed
mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
@gibson042 @dmethvin
Fix #12229, size/consistency improvements. Close jquerygh-887.
7520927
@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gibson042 @dmethvin @rwaldron @Krinkle @jaubourg