Subscription interface: closed method

[lucamezzalira]

Hi All

reviewing the proposal I'd suggest changing the closed method name to isClosedthat would suggest immediately that the method will return a boolean value.

[appsforartists]

It's a getter, not a method.

[ljharb]

In that case, can it be a method, not a getter? Getters are slow, and foo.x === foo.x not always being guaranteed to be true is weird (not sure if that's a possibility here).

[appsforartists]

get closed() : Boolean;

A Boolean should always equal an equivalent Boolean, so I don't think your concerns about === are relevant here.

"Getters are slow" sounds like premature optimization, especially without a citation. Can you imagine a realistic situation where the difference in speed between a method and a getter would be meaningful with regard to isClosed vs isClosed()?

[lucamezzalira]

@appsforartists Sorry my bad, but the question still stand, don't you think would be more clear using isClosed instead of closed?

[RangerMauve]

It might be good to look at other JS/DOM APIs to see if isBoolean is a common trait. AFAIK, prefixing methods that return booleans is more of a Java thing and I don't recall seeing that in any DOM/Node APIs (other than in libraries).

[lucamezzalira]

Cluster module: https://nodejs.org/api/cluster.html#cluster_cluster_ismaster https://nodejs.org/api/cluster.html#cluster_worker_isdead https://nodejs.org/api/cluster.html#cluster_cluster_isworker https://nodejs.org/api/cluster.html#cluster_worker_isconnected https://nodejs.org/api/cluster.html#cluster_worker_isdead

Buffer module https://nodejs.org/api/buffer.html#buffer_class_method_buffer_isbuffer_obj https://nodejs.org/api/buffer.html#buffer_class_method_buffer_isencoding_encoding

Path module: https://nodejs.org/api/path.html#path_path_isabsolute_path

Array: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray

I can go ahead if you prefer, prefixing is it's not a Java thing, it's a best practice in many languages. In fact, also in javascript is used extensively across several APIs

[RangerMauve]

Are there any plain old JS or DOM APIs that also match this? The reason I bring that up is that Observables aren't made to integrate with the node ecosystem per-se, but for JavaScript as a whole. And the APIs in browsers are a big part of that.

Array.isArray() shouldn't count because it's not a method returning the current state of an object, it's checking the type, similar for isBuffer in node.

The cases in the cluster module are compelling since these are getters that return the state of an object. It'd be cool if someone also had counter examples for getters returning the boolean state of an object.

[aruns07]

I mostly find myself using isSomethingCurrent or hasSomethingDone style of names.

Reading this issue made to think that

• hasSomethingDone and somethingDone both tells about an event/action has happened or not in the past.
• isSomethingCurrent and somethingCurrent tells about the current property assumption of an object.

Well English is not my first language, please correct me if following knowledge is not correct.

I think we could consider Noun, Adjective, and Verb to decide.

has[Past Verb], [Past Verb], is[Noun], is[Adjective], [Adjective] is suitable for boolean return.

hasClosed, closed sounds same and dropping prefix is better.

[mattflix]

@appsforartists wrote:

A Boolean should always equal an equivalent Boolean, so I don't think your concerns about === are relevant here.

@appsforartists, you may be surprised to see what gets logged to the console by the following code:

console.log(new Boolean(true) === true);
console.log(new Boolean(false) === false);

The closed getter on SubscriptionObserver should really be defined to return a boolean primitive, not an instance of the Boolean object type (which is pretty big footgun, and seems like an obvious mistake to me, or at least a typo that will be very important to fix).

If the current API proposal is left unchanged, it means users will have to workaround the following rather surprising behavior:

    if (subscription.closed) {
        // always executes, since closed is always a truthy value
    }

See also #184.

[benlesh]

@mattflix that seems like a typo, honestly.