Improve heartbeat queue checking

[evansolomon]

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).

[nacin]

Looks good at a glance. Thanks evan.

[azaozz]

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.

[azaozz]

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

[evansolomon]

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

[azaozz]

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.

[azaozz]

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.

[evansolomon]

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.

[azaozz]

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.

[evansolomon]

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;
}

[azaozz]

It should still check isQueued() first...

Yep, see 25047-4.patch.

[evansolomon]

Cool, looks good

[azaozz]

In 25078:

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