WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 8 months ago

#25970 closed defect (bug) (fixed)

jshint shouldn't throw errors: wp-includes/js/admin-bar.js

Reported by: kadamwhite Owned by: nacin
Milestone: 3.8 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description

Aside from onevar fixes, this file has two JSHint directive changes: the inline'd jQuery hoverIntent plugin is ignored, and I have disabled loopfunc for this file.

The purpose of "loopfunc": true is to prevent dangerous situations where defining a function within a loop yields unexpected results. From the JSHint docs, this is the antipattern example:

var nums = [];

for (var i = 0; i < 10; i++) {
  nums[i] = function (j) {
    return i + j;
  };
}

nums[0](2); // Prints 12 instead of 2

The issue is that i needs to be copied into an immediately-invoked function in order for the variable reference to be correctly interpreted:

var nums = [];

for (var i = 0; i < 10; i++) {
  (function (i) {
    nums[i] = function (j) {
        return i + j;
    };
  }(i));
}

The specific code that was yielding function-within-loop errors is all avoiding this error, either by wrapping the relevant function definition in an IIFE or by keeping all function calls and references within the same loop context in which the function is defined. Because we're in an environment (the site front-end) where we want to keep file size down and therefore cannot reliably count on either Underscore or jQuery's iterators, I feel that in this file we're being responsible enough to mute these errors.

Attachments (1)

25970.diff (2.5 KB) - added by kadamwhite 8 months ago.
Changes to make grunt jshint:core --file=admin-bar.js pass without error

Download all attachments as: .zip

Change History (6)

kadamwhite8 months ago

Changes to make grunt jshint:core --file=admin-bar.js pass without error

comment:1 follow-up: kadamwhite8 months ago

  • Cc kadamwhite@… added

See #14772 for the genesis story of this file. I'm not 100% sure whether the loopfunc-related code is even originally first-party, but it's been around for a while.

comment:2 in reply to: ↑ 1 ; follow-up: nacin8 months ago

Replying to kadamwhite:

See #14772 for the genesis story of this file. I'm not 100% sure whether the loopfunc-related code is even originally first-party, but it's been around for a while.

It's HoverIntent, bundled here to avoid the extra HTTP request. If jQuery is available (as provided by the theme or a plugin), the admin bar uses jQuery + HoverIntent. If it isn't , it uses its own raw JS. Fun times...

comment:3 nacin8 months ago

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

In 26160:

Fix JSHint errors in admin-bar.js.

props kadamwhite.
fixes #25970.

comment:4 nacin8 months ago

  • Milestone changed from Awaiting Review to 3.8

comment:5 in reply to: ↑ 2 nacin8 months ago

Replying to nacin:

Replying to kadamwhite:

See #14772 for the genesis story of this file. I'm not 100% sure whether the loopfunc-related code is even originally first-party, but it's been around for a while.

It's HoverIntent, bundled here to avoid the extra HTTP request. If jQuery is available (as provided by the theme or a plugin), the admin bar uses jQuery + HoverIntent. If it isn't , it uses its own raw JS. Fun times...

Sorry, misread. I don't know where it came from. My hunch is filosofo wrote it.

Note: See TracTickets for help on using tickets.