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

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
5 participants
Member

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

"Please use with caution" ==> "Some incredibly common plugin probably uses this without asking and will break horribly when we remove it."

Can we save that pain for 1.9? I'd feel bad doing this in a minor-point release, even on an undocumented property. We can still switch to guid for everything internally.

Neither is a great example of clear code, but I can stumble through the old code better than the new. I have to admit it's a clever way to do it though.

Member

gibson042 commented Aug 20, 2012

Good points as usual, @dmethvin.

Member

gibson042 commented Sep 18, 2012

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

Owner

dmethvin commented Sep 18, 2012

Was just working on this, yeah.

@rwaldron rwaldron commented on the diff Sep 18, 2012

src/data.js
@@ -232,7 +232,7 @@ jQuery.fn.extend({
for ( l = attr.length; i < l; i++ ) {
name = attr[i].name;
- if ( name.indexOf( "data-" ) === 0 ) {
+ if ( !name.indexOf( "data-" ) ) {
@rwaldron

rwaldron Sep 18, 2012

Member

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

@dmethvin

dmethvin Sep 18, 2012

Owner

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

@rwaldron

rwaldron Sep 18, 2012

Member

it's a nested loop...?

@jaubourg

jaubourg Sep 18, 2012

Member

I love hot paths.

@rwaldron

rwaldron Sep 18, 2012

Member

I love hot pants

@rwaldron

rwaldron Sep 18, 2012

Member

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!

@dmethvin

dmethvin Sep 18, 2012

Owner

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

@dmethvin dmethvin commented on the diff Sep 18, 2012

src/data.js
@@ -58,7 +58,7 @@ jQuery.extend({
// Only DOM nodes need a new unique ID for each element since their data
// ends up in the global cache
if ( isNode ) {
- elem[ internalKey ] = id = jQuery.deletedIds.pop() || ++jQuery.uuid;
+ elem[ internalKey ] = id = jQuery.deletedIds.pop() || jQuery.guid++;

dmethvin closed this in 15b5dbf Sep 18, 2012

Owner

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.

How does that save bytes, its the same byte count? Or do you mean through sampleability through gzip?

Owner

gibson042 replied Sep 21, 2012

Exactly. We try to gzip as small as possible.

@mescoda mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014

@gibson042 @dmethvin gibson042 + dmethvin Fix #12229, size/consistency improvements. Close gh-887. 7520927
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment