Subscription#closed should return boolean, not Boolean

[mattflix]

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
    }

[dinoboff]

The specs don't mention returning a Boolean object:

https://tc39.github.io/proposal-observable/#subscription-prototype-object

[mattflix]

That's great.

By "spec", I meant the text of the proposal (right next the bottom, where the SubscriptionObserver interface is shown), here: https://github.com/tc39/proposal-observable/blob/master/README.md

It's good to see that the typo (I guess) didn't make it into the formal spec.

There should still probably fix the typo, as it may cause unnecessary confusion/angst for anybody coming to the README to become familiar with the details of the proposal. The use of Boolean as the return type is misleading.

[ljharb]

I don’t think it’s a typo. In every current type system I’ve seen in JS, Boolean refers to the primitive, and nobody considers the existence of a boxed Boolean object at all.

[mattflix]

@ljharb, in Flow, the following code does not compile:

function g(x: Boolean) {}
g(true); // ERROR

// g(true);
//   ^ boolean. This type is incompatible with the expected param type of
// function g(x: Boolean) {
//               ^ Boolean

In TypeScript, too, boolean and Boolean are two different types with completely different semantics.

Again, this is probably a minor point, and really just a typo.

The important thing is that the formal spec (referred to by @dinoboff above) correctly shows that closed yields a boolean primitive.

So, this issue can be closed, athough it would be nice if the informal README was corrected from "Boolean" to "boolean".

[ljharb]

I wasn’t aware of that; thanks for clarifying. In that case it should indeed be the lowercase version.

[benjamingr]

I'm +1 on keeping the existing usage as it is what the spec uses and this is a spec repo.

[mattflix]

@benjamingr, not clear what you're advocating.

You are suggesting that Boolean be taken to mean boolean?