Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

WIP: perf(jqLite): use classList for add/removeClass #7829

Closed
wants to merge 2 commits into from

Conversation

rodyhaddad
Copy link
Contributor

Continuation of #7759
Using classList.apply caused problems in IE 10/11.

I believe we still want these changes for performance, but not yet.
So keeping them here for being considered later.

IgorMinar and others added 2 commits June 13, 2014 10:17
All elements/browsers that don't have classList will still use the old setTimeout method.
All elements/browsers that don't have classList will still use the old setAttribute method
@caitp
Copy link
Contributor

caitp commented Jun 13, 2014

@mzgol would know this better than me, but I'm fairly certain I recall jQuery opting to avoid classList because it hurt performance rather than helped it, in addition to requiring multiple code paths.

I'm not sure this is actually a performance "fix", possibly a regression, although it's not likely that we can actually measure a significant difference given that class operations are not super common

@caitp
Copy link
Contributor

caitp commented Jun 13, 2014

and yeah, (old) opera (~v12-ish) and possibly old IE browsers don't deal with the multiple args to classList apis correctly :(

@rodyhaddad
Copy link
Contributor Author

This is here because we won't merge them from #7759 for the next release.

We might consider not merging them at all later on.

@lgalfaso
Copy link
Contributor

Is this the same as #7759 9db0cdf ?

@lgalfaso
Copy link
Contributor

@rodyhaddad you beat me while I was writing my comment :)

@rodyhaddad
Copy link
Contributor Author

@lgalfaso Yes, didn't want to lose track of it :-)

@rodyhaddad rodyhaddad changed the title perf(jqLite): use classList for add/removeClass WIP: perf(jqLite): use classList for add/removeClass Jun 13, 2014
@caitp
Copy link
Contributor

caitp commented Jun 13, 2014

anyways, I am not big on having piles of paths for this stuff, if it's going to be done, we should decide which path to take before binding jqLite (and this kind of sucks for maintenance, too)

@rodyhaddad rodyhaddad added this to the 1.3.0-beta.14 milestone Jun 16, 2014
@IgorMinar IgorMinar closed this Jun 30, 2014
@IgorMinar
Copy link
Contributor

we are going to skip this one until IE has better support

@mgol
Copy link
Member

mgol commented Jul 29, 2014

Sorry to be late for the party but yeah, apart from IE9 not supporting classList which even jQuery 2 still supports performance is not better. The confusion might come from the fact that classList performance in some browsers (like Chrome) was better for some time but that was when only a single className argument was supported. Once support for multiple arguments was added, performance went back to jQuery levels (I suppose nothing really more performant can happen natively than what jQuery currently does).

classList is definitely a much nicer interface for people that need to type it frequently by hand but for libraries, where you write once and use a lot it doesn't create any significant advantage (and it introduces some drawbacks like not being able to control spacing/trimming of the className property value etc.). And it's certainly not worth introducing two code paths.

IMO it'll make sense to switch to classList once all supported browsers support it fully (i.e. by accepting multiple params, not one like IE right now) but not before.

@rodyhaddad
Copy link
Contributor Author

@mzgol Thanks for all the info!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants