Handling iteration over inherited properties before own properties. Fixes #12199 #1196

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
9 participants

bjohn465 commented Mar 6, 2013

I added an iteratesOwnLast property to jQuery.support to check for browsers that iterate over inherited properties before own properties, which only seems to be IE < 9. I used the same technique that lowdash uses.

In jQuery.isPlainObject, I also followed the technique in lowdash to handle iteratesOwnLast browsers.

Let me know how it looks.

Owner

gnarf commented Mar 6, 2013

👍 seems sane

Owner

gnarf commented Mar 6, 2013

cc @jdalton @rwldrn - Review please?

Member

jdalton commented Mar 6, 2013

I can't speak to the code style/var-name use, but dig the implementation. That reminds me I need to expose that stuff on a support property in lodash as well ;)

Member

jdalton commented Mar 6, 2013

Also note Closure Compiler in "advanced" mode may remove b as dead code. I had to correct it here. Not a knock against it, it's just super happy om-nom'ing what it thinks are dead bits of code and smth to be aware of.

src/support.js
+ for( i in new func() ) {
+ props.push( i );
+ }
+ support.iteratesOwnLast = props[0] !== "a";
@gibson042

gibson042 Mar 6, 2013

Member

This whole block can be dramatically reduced by using jQuery( div ) as the constructor, which will have a context property not appearing in jQuery.fn.

src/core.js
+ // Support: IE<9
+ // Handle iteration over inherited properties before own properties.
+ if ( jQuery.support.iteratesOwnLast ) {
+ for( key in obj ) {
@timmywil

timmywil Mar 6, 2013

Owner

for( => for (

src/support.js
@@ -1,6 +1,7 @@
jQuery.support = (function( support ) {
- var all, a, input, select, fragment, opt, eventName, isSupported, i,
+ var all, a, input, select, fragment, opt, eventName, isSupported, i, func,
@rwaldron

rwaldron Mar 6, 2013

Member

remove func from this list

src/support.js
@@ -231,7 +246,7 @@ jQuery.support = (function( support ) {
});
// Null elements to avoid leaks in IE
- all = select = fragment = opt = a = input = null;
+ all = select = fragment = opt = a = input = func = props = null;
@rwaldron

rwaldron Mar 6, 2013

Member

remove func from this list

Member

rwaldron commented Mar 6, 2013

@gibson042 can you come up with a better (ie. gzip-friendly) name for jQuery.support.iteratesOwnLast?

Member

rwaldron commented Mar 6, 2013

@bjohn465 If you haven't already, please sign: http://jquery.github.com/cla.html

Member

jdalton commented Mar 6, 2013

@rwldrn ownLast ? (last implies order still)

Member

rwaldron commented Mar 6, 2013

@jdalton that's exactly what I was thinking, but @gibson042 has a particularly savvy knack for this kind of thing ;)

Member

gibson042 commented Mar 6, 2013

I'd go for prototypeFirst, which saves about 7 bytes over iteratesOwnLast (but just one over ownLast).

Member

rwaldron commented Mar 6, 2013

@jdalton see? 😉

Member

gibson042 commented Mar 6, 2013

(not checked: savings of moving the assignment before element-based tests)

Member

jdalton commented Mar 6, 2013

prototypeFirst has less meaning (it's own vs inherited), prototypeFirst makes me think like its the prototype property thats it first (say iteratin the jQuery function).

Member

rwaldron commented Mar 6, 2013

I'd be happy trading the one byte for ownLast

Member

jdalton commented Mar 6, 2013

I know jQuery is concerned with file size, so I think it would benefit the project if they took the approach to minification that Lo-Dash takes.

Lo-Dash will do a pass with Closure Compiler "simple" mode, then with Closure Compiler "advanced", then with UglifyJS, then a hybrid pass of Closure Compiler "simple" + UglifyJS, and finally a hybrid pass of Closure Compiler "advanced" + UglifyJS. It then picks the smallest gzipped minification for its output.

This helps because different builds or versions of Lo-Dash will minify better with different approaches. For example in Lo-Dash normally Closure Compiler "advanced" + UglifyJS produces the smallest file, but for the lodash underscore build Closure Compiler "simple" + UglifyJS is the smallest. With something like this you all wouldn't have to worry as much.

Member

rwaldron commented Mar 6, 2013

@jdalton I love you, but that sounds insane 😉

Member

jdalton commented Mar 6, 2013

Just saying, it works, is auto-magic, and saves time because I don't have to pore over the source agonizing over property names or other minification jazz. Crazy like a fox :)

Member

jdalton commented Mar 6, 2013

I should also mention it usually gives me a 250+ byte win w/o any extra effort.

Owner

timmywil commented Mar 6, 2013

@jdalton : why not do both? MOAR BYTES EATEN.

Member

jdalton commented Mar 6, 2013

@timmywil I'm trying to hide my crazy. I did dig further for like the lodash underscore build because I wanted it closer to Underscore's size, but in my day to day I don't have to worry. I also fix the minified output to protect against slow paths minifiers introduce into code as well (that's the crazy coming through :P).

Member

gibson042 commented Mar 6, 2013

I also fix the minified output to protect against slow paths minifiers introduce into code as well (that's the crazy coming through :P).

We're not acting on it yet, but that's a crazy I can relate to!

Member

jdalton commented Mar 6, 2013

@gibson042 Ah nice, btw that's been fixed in UglifyJS2. I do things that like in the minification/build step to avoid polluting the dev source with minification escapes, tweaks, and other build related things.

Owner

timmywil commented Mar 6, 2013

@jdalton : No need to hide your crazy here. :)

bjohn465 commented Mar 7, 2013

@rwldrn I've signed the CLA before for my contributions to jQuery Mobile, so I should be good. If not, I can sign it again.

I streamlined the way ownLast is detected, since I didn't want to iterate over all the properties of the jQuery object. Let me know how the changes look.

src/core.js
+ // Support: IE<9
+ // Handle iteration over inherited properties before own properties.
+ if ( jQuery.support.ownLast ) {
+ for( key in obj ) {
@Krinkle

Krinkle Mar 8, 2013

Member

Space after for.

src/support.js
@@ -141,6 +142,13 @@ jQuery.support = (function( support ) {
div.cloneNode( true ).style.backgroundClip = "";
support.clearCloneStyle = div.style.backgroundClip === "content-box";
+ // Support: IE<9
+ // Iteration over object's inherited properties before its own.
+ for ( i in $div ) {
@rwaldron

rwaldron Mar 8, 2013

Member

Why not:

for ( i in jQuery( div ) ) {
  break;
}
@bjohn465

bjohn465 Mar 8, 2013

Because of line 150 where I check to see if the first property is an ownProperty of the jQuery object. I didn't want to jQuery( div ) twice.

@bjohn465

bjohn465 Mar 8, 2013

@rwldrn I suppose I could do support.ownLast = i !== "0"; on line 150 instead, then I don't have to keep a reference to jQuery( div ) around. Do you like that better?

@bjohn465

bjohn465 Mar 8, 2013

Although, that seems a little more fragile to me, as the current method would work even if "0" is ever not the first own property. I suppose if that happened, though, a unit test would fail (hopefully).

@rwaldron

rwaldron Mar 8, 2013

Member

Because of line 150 where I check to see if the first property is an ownProperty of the jQuery object. I didn't want to jQuery( div ) twice.

Yeah, I missed that part.

src/support.js
@@ -1,7 +1,8 @@
jQuery.support = (function( support ) {
var all, a, input, select, fragment, opt, eventName, isSupported, i,
- div = document.createElement("div");
+ div = document.createElement("div"),
+ $div = jQuery( div );
@gnarf

gnarf Mar 19, 2013

Owner

do we use this $div style anywhere else? I don't think we do... Perhaps wrapped ?

@timmywil

timmywil Mar 19, 2013

Owner

This style is actually in our API style guide. We don't need this a lot in core, but it would be nice if the style guides had the same rule on this.

@scottgonzalez

scottgonzalez Mar 19, 2013

Owner

Can we remove it from the API style guide? Where is that anyway? It goes against the UI style guide.

@timmywil

timmywil Mar 19, 2013

Owner

It should probably stay. It is more readable. Still, the API guide has other things that are better suited for online documentation (such as 2 spaces instead of tabs to avoid horizontal scrollbars). So I guess all the guides don't need to agree. In this particular case, I suppose it's best for core to adopt UI's guide.

@timmywil

timmywil Mar 19, 2013

Owner

Actually, I'm surprised UI has a separate style guide at all. Seems like the "Javascript Style Guide" could cover all of our javascript projects. http://contribute.jquery.org/style-guide/js/

@timmywil

timmywil Mar 19, 2013

Owner

I see, it just adds to the standard guide. Sorry for the confusion. http://wiki.jqueryui.com/w/page/12137737/Coding-standards.

@timmywil

timmywil Mar 19, 2013

Owner

Except....I don't see anything about jQuery-wrapped variables in the UI guide.

@rwaldron

rwaldron Mar 19, 2013

Member

I want to delete this entire thread.

@bjohn465, can you do me a favor and try this:

  1. Remove the $div declaration
  2. Replace the for-in with this:
for ( i in jQuery({}) ) {
  break;
}
support.ownLast = i !== "0";
@gibson042

gibson042 Mar 19, 2013

Member

@rwldrn +1 (or rather, roughly -10).

...and then -1 more by switching to jQuery( support ).

I think we're done here.

@rwaldron

rwaldron Mar 20, 2013

Member

@bjohn465 To recap, that's:

for ( i in jQuery( support ) ) {
  break;
}
support.ownLast = i !== "0";

thanks!

@dmethvin dmethvin closed this Apr 4, 2013

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment