WordPress.org

Make WordPress Core

#25047 closed defect (bug) (fixed)

Improve heartbeat queue checking

Reported by: evansolomon Owned by: azaozz
Milestone: 3.7 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: Cc:

Description

The JS heartbeat API has an isQueued() method, but it also checks whether a given handle is already queued in its enqueue() method (without using isQueued()). Unfortunately the checks are not quite equal. Currently isQueued() will return (likely) incorrect truthy values if you do something like wp.heartbeat.isQueued('constructor'), or the somewhat ironic wp.heartbeat.isQueued('hasOwnProperty').

  1. The enqueue() method should use the publicly available isQueued() for consistency
  2. The isQueued() method should use hasOwnProperty()

I also rearranged the order of the queue check in enqueue() to check dont_overwrite before the checking the queue. The dont_overwrite param is falsey by default, so we might as well check that before doing the more expensive operation of checking the queue (we don't care if something is in the queue if we're going to overwrite it anyway).

Attachments (4)

25047.patch (711 bytes) - added by evansolomon 22 months ago.
25047-2.patch (911 bytes) - added by azaozz 21 months ago.
25047-3.patch (1.1 KB) - added by azaozz 21 months ago.
25047-4.patch (1.4 KB) - added by azaozz 21 months ago.

Download all attachments as: .zip

Change History (16)

@evansolomon22 months ago

comment:1 @nacin22 months ago

  • Milestone changed from Awaiting Review to 3.7
  • Owner set to azaozz
  • Status changed from new to assigned

Looks good at a glance. Thanks evan.

comment:2 @azaozz21 months ago

Yep, the idea sounds good. There's a typo queue.isQueued should be this.isQueued. Also there is no need to do

if (! queue.hasOwnProperty( handle )) 
    return null;

As queue is a simple object, return queue[handle]; returns null if handle doesn't exist in queue. Of course using JS "reserved words" may trigger an error or return wrong value.

Last edited 21 months ago by azaozz (previous) (diff)

@azaozz21 months ago

comment:3 follow-up: @azaozz21 months ago

On the other hand wp.heartbeat.isQueued() should probably return 'false' for consistency with wp.heartbeat.enqueue().

comment:4 in reply to: ↑ 3 ; follow-up: @evansolomon21 months ago

Replying to azaozz:

On the other hand wp.heartbeat.isQueued() should probably return 'false' for consistency with wp.heartbeat.enqueue().

This makes sense to me.

As for the original version, it would have returned something null-y, but not actually null.

var obj = {};
obj.foo === null;
// false

obj.foo == null;
// true

obj.foo === undefined;
// true

comment:5 in reply to: ↑ 4 @azaozz21 months ago

Replying to evansolomon:

As for the original version, it would have returned something null-y, but not actually null.

Ah yes, one of the "great mysteries" in JS :)

undef1 // throws "ReferenceError: undef1 is not defined"
typeof undef1 // 'undefined'

var undef2;
undef2 // no error
typeof undef2 // 'undefined'

var obj = {};
obj.undef3 // no error
typeof obj.undef3 // 'undefined'

So undef1 is really really undefined. Both undef2 and undef3 are still undefined but not as much undefined as undef1. They behave similarly to being set to null, however JS wouldn't usually set anything explicitly to null.

@azaozz21 months ago

comment:6 @azaozz21 months ago

Thinking more about this: there is a good user case to set a queued item to false. There is even a case to set it to null. If we return false or null from wp.heartbeat.isQueued() and use that function in wp.heartbeat.enqueue() we will miss either one or the other.

In addition, obj.hasOwnProperty() works pretty much like isset() in PHP, i.e. it doesn't care what the value is, only checks whether the property exists. So it would be best to use hasOwnProperty directly in wp.heartbeat.enqueue() and return undefined from wp.heartbeat.isQueued().

Also added wp.heartbeat.dequeue() in 25047-3.patch.

Last edited 21 months ago by azaozz (previous) (diff)

comment:7 @evansolomon21 months ago

I'll propose another idea.

  • isQueued() should only return a boolean, never the queued value, null, etc. This can just be this.isQueued = queue.hasOwnProperty
  • There should be a separate method (getQueuedItem() or something) to fetch a queued value, which would return the value if isQueued() is true. I'm not sure what it should return when isQueued() is false (false, null, undefined, "LOL JavaScript"...pick your favorite).

This lets enqueue() check the queue in the same way everyone else can and eliminates any confusion about false-y values.

comment:8 follow-up: @azaozz21 months ago

Was actually thinking the same: isQueued() can return only boolean. Didn't want to change how it works but may be worth it. Then getQueuedItem() can directly return queue[ handle ], i.e. undefined if handle is not in queue.

comment:9 in reply to: ↑ 8 @evansolomon21 months ago

Replying to azaozz:

getQueuedItem() can directly return queue[ handle ], i.e. undefined if handle is not in queue.

It should still check isQueued() first to avoid cases like wp.heartbeat.isQueued('constructor'). For example:

this.getQueuedItem = function(item) {
  return this.isQueued(item) ? queue[item] : undefined;
}

@azaozz21 months ago

comment:10 @azaozz21 months ago

It should still check isQueued() first...

Yep, see 25047-4.patch.

comment:11 @evansolomon21 months ago

Cool, looks good

comment:12 @azaozz21 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 25078:

Heartbeat: better queue functionality: improve enqueue() and isQueued(), introduce dequeue() and getQueuedItem(). Props evansolomon, fixes #25047.

Note: See TracTickets for help on using tickets.