Skip to content

Fix cross-browser object enumeration issues#385

Merged
jashkenas merged 1 commit into
jashkenas:masterfrom
jdalton:dontenum
Dec 2, 2011
Merged

Fix cross-browser object enumeration issues#385
jashkenas merged 1 commit into
jashkenas:masterfrom
jdalton:dontenum

Conversation

@jdalton

@jdalton jdalton commented Dec 1, 2011

Copy link
Copy Markdown
Contributor

Fix cross-browser object enumeration issues.

This patch should address the concerns brought up by @jashkenas in issue #376.

The gzipped filesize diff is around ~0.23 kb and the speed concerns have been addressed (will add perf tests asap) by optimizing other areas of underscore.js.

As I mentioned in the original issue several-libs-attempt to tackle this issue and recently @kangax tweeted:

Got bitten by function's "prototype" being (incorrectly) enumerable in Opera <12 (copying static properties also overwrote instance ones) :)

Currently it passes the existing unit tests (one throttle unit test fails in IE6, but it does on the official release too).

If this patch gets the green light I will commit unit tests and further patches/pull-requests to speed up underscore.js methods by up to 50%.

@ghost

ghost commented Dec 1, 2011

Copy link
Copy Markdown

👍

@seidtgeist

Copy link
Copy Markdown

+1

@jashkenas

Copy link
Copy Markdown
Owner

I'd love to see the JSPerf.com tests, comparing this for some simple iterations of thousand-length arrays and objects.

@jdalton

jdalton commented Dec 2, 2011

Copy link
Copy Markdown
Contributor Author

The perf impact seems to be minimal either fluctuating between being equal or the pull request 385 being ~5-9 percent slower from refresh to refresh. The wiggle room for each testing being marked indistinguishable is ~5.5 percent so things are very close and in IE6 it seems the pull request iteration is actually faster despite the extra checks (because of other optimizations). This patch addresses enumeration issues in IE < 9, Safari < 5.1 and Opera < 12.

jashkenas added a commit that referenced this pull request Dec 2, 2011
@jashkenas jashkenas merged commit 3c6a8cc into jashkenas:master Dec 2, 2011
@jashkenas

Copy link
Copy Markdown
Owner

Merged -- if you'd like to tweak some of these additions to read more clearly, feel free. Otherwise I may take a stab at it later.

@jdalton

jdalton commented Dec 2, 2011

Copy link
Copy Markdown
Contributor Author

K, I will do a little cleanup when I add the unit tests in a pull request this evening.
Also I will get to work on the patch to increase method perf up to %50 :D

@jashkenas

Copy link
Copy Markdown
Owner

Probably.

@jdalton

jdalton commented Dec 6, 2011

Copy link
Copy Markdown
Contributor Author

Now that all the commits have been ironed out the perf of edge is faster or equal to v1.2.2 :D

@jashkenas

Copy link
Copy Markdown
Owner

That's rad.

@jashkenas

Copy link
Copy Markdown
Owner

The above commit rolls back the changes introduced in this ticket. With all due apologies for temporarily having them on master -- after further discussion, implementing such significant code branches to solve a problem that's so rarely encountered in real-world code isn't a direction that Underscore should take.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants