Skip to content
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.

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

avoid unnecessary HTML string building+parsing #16024

Merged
merged 1 commit into from
Mar 19, 2015
Merged

Conversation

cvrebert
Copy link
Collaborator

@cvrebert cvrebert commented Mar 9, 2015

Rather than building a string of HTML and then parsing it, which is overkill and comparatively slow, this simply creates a new blank <div> and then adds the requisite classes via jQuery.
This could be further tweaked to use className instead of jQuery's addClass at the cost of a bit more verbosity; I opted to be conservative.

@cvrebert cvrebert added the js label Mar 9, 2015
@cvrebert
Copy link
Collaborator Author

CC: @hnrch02 @XhmikosR

@XhmikosR
Copy link
Member

I couldn't find any references that this is faster. Do you have any numbers? Also, are these the only cases?

The idea seems fine to me, just wondering.

@cvrebert
Copy link
Collaborator Author

Do you have any numbers?

Yes: http://jsperf.com/dom-constr-42
jsperf
firefox
(Firefox won't upload due to bad CSRF token problem for some reason.)

Also, are these the only cases?

Yes, except for the unit tests.

@XhmikosR
Copy link
Member

Wow, huge difference! Definitely :shipit: .

How did you spot this?

@cvrebert
Copy link
Collaborator Author

I was reminded about it when nitpicking an instance of it in some new unit tests in a PR the other day.

@cvrebert cvrebert modified the milestone: v3.3.5 Mar 15, 2015
cvrebert added a commit that referenced this pull request Mar 19, 2015
avoid unnecessary HTML string building+parsing
@cvrebert cvrebert merged commit 9aad9a4 into master Mar 19, 2015
@cvrebert cvrebert deleted the unnecessary-parsing branch March 19, 2015 05:28
@cvrebert cvrebert mentioned this pull request Mar 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants