Make WordPress Core

Opened 10 years ago

Closed 9 years ago

#27034 closed enhancement (fixed)

jQuery methods html('') vs empty()

Reported by: madeinua's profile madeinua Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.8.1
Component: Administration Keywords: has-patch
Focuses: javascript, performance Cc:

Description (last modified by TobiasBg)

I found that in a many places in WP uses .html('') instead of .empty()

There is a few advantages to use the second one:

  1. $.empty() works faster
  2. $.html('') creates memory leak
  3. $.empty() is more readable

Confirmation of these words you can find on the internet (e.g. stackoverflow).

Thanks!

Attachments (2)

27034.patch (5.1 KB) - added by SergeyBiryukov 10 years ago.
27034.diff (5.4 KB) - added by OriginalEXE 10 years ago.

Download all attachments as: .zip

Change History (13)

#1 @TobiasBg
10 years ago

  • Component changed from Appearance to Administration
  • Description modified (diff)

#2 @SergeyBiryukov
10 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.9
  • Type changed from feature request to enhancement

#3 @SergeyBiryukov
10 years ago

According to jQuery documentation, empty() also removes event handlers from child elements, so we should verify that doesn't affect any of the instances here.

#4 @madeinua
10 years ago

true, in this cases you can use .detach()

#5 @nacin
10 years ago

  • Milestone changed from 3.9 to Future Release

In order to touch this we'd have to ensure there are no side effects for any one of these changes.

#6 @OriginalEXE
10 years ago

Removing events is only 'dangerous' if removed elements are going to be returned to the dom.

From my inspection, this does not happen anywhere. The only place where object is returned (and where events could potentially break) when .html('') is used is in wp-ajax-response.js, so just to be sure, i used detach() there instead.

@OriginalEXE
10 years ago

#7 @wonderboymusic
9 years ago

  • Milestone changed from Future Release to 4.2

#8 follow-up: @helen
9 years ago

We may not be set up to handle this right now, but just a note for now/the future that it would be nice if this was testable in some way, particularly in terms of performance.

#9 in reply to: ↑ 8 @adamsilverstein
9 years ago

Replying to helen:

We may not be set up to handle this right now, but just a note for now/the future that it would be nice if this was testable in some way, particularly in terms of performance.

Here is some jsPerf data comparing speed for empty(), html() and native .innerHTML =

http://jsperf.com/empty-vs-html/2

#10 @wonderboymusic
9 years ago

I just looked at every line these touch, they are all arbitrary uses of .html('')

#11 @wonderboymusic
9 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 31690:

Replace flagrant instances of .html('') with .empty().

Props OriginalEXE, SergeyBiryukov.
Fixes #27034.

Note: See TracTickets for help on using tickets.