Bad input to subscribe (no args, non-callable notifications) should not interfere with subscription.

[jhusain]

--edited for clarity and grammatical fixes-- The current specification calls for Observable.prototype.subscribe to throw if no arguments are passed to it.

someObservable.subscribe() // throws in current spec

This seems both counter-productive and inconsistent with Promises. It is counter-productive, because unlike Promise.prototype.then calling Observable.prototype.subscribe may cause side-effects. It's perfectly reasonable for a developer to want trigger side-effects and ignore the results of these side effects.

Furthermore throwing on no arguments or bad arguments is inconsistent with Promises. Promises don't throw in any of the following situations.

Promise.resolve(5).then()
Promise.resolve(5).then("not a callback")

Furthermore in both of the examples above, the result is not a rejected Promise, and HostReportErrors is not notified. It seems there is precedent in JS for ignoring a failure on the consumer's part to correctly specify any reasonable way of receiving the result of a push notification. Furthermore, such a failure is not conflated with a producer failure.

I propose that invalid input should not cause subscribe to throw, including passing a non-callable argument instead of a callback, or having a non-callable value as an Observer method. In short, if a consumer does not specify any reasonable way of observing the results of a subscribe operation, that should interfere with the side-effects the subscribe method is attempting to perform.

[benjamingr]

Promises were forced to behave this way in order to be compatible with jQuery.

No one (that I'm aware of) likes this behavior and we were mostly stuck with it in order to be compatible with the existing ecosystem.

Promises are wrong here - let's not repeat the same mistakes.

[jhusain]

That's great context @benjamingr. Do you have a link I can read re: this issue? Need a pretty strong argument to get around consistency here.

Any comment on the utility of subscribe with no arguments?

[jhusain]

@domenic would be interested in your comments here.

[satya164]

While consistency is nice, less error checking sounds like a bad idea. Say subscribe doesn't throw error on invalid argument, I might pass a wrong variable as argument or make a you're typo and pull my hair all day long to know why it's not working. However, if I just wanna ignore the result I could just pass a noop listener.

Please don't sacrifice error checking just to be consistent with promises.

[jhusain]

Definitely sympathetic to those who want strong error-checking. However I need to understand the rationale for the promise design decision. Looking through the minutes of the meetings to see if I can find one.

[appsforartists]

Accepting an empty subscribe() to pull values through the stream sounds reasonable, but not notifying the author of invalid input does not.

subscribe(12345) should throw an error. subscribe({ notNamedNext(){} })probably should too.

[benjamingr]

@jhusain https://github.com/promises-aplus/promises-spec/issues/25 but I recall a lot of discussion at #promises on IRC back then.

[benjamingr]

Also, note https://github.com/promises-aplus/promises-spec/issues/25#issuecomment-10424753

promise.then(null, function () {})

was needed since .catch wasn't part of a spec, so you had to have some ignored value. Also this:

Interestingly, jQuery seems to swallow strings for optional function arguments in jQuery.ajax. I suspect this is just an issue people don't often notice. Args tend to be passed with certainty and it tends to be obvious what sort of arg should be passed.

Lots of discussion there - but mostly I think its sums up to:

• Standardizing promises libraries - which were doing this (in A+, and then the spec).
• No strong arguments either way.
• Doing .then(null, something) since .catch isn't part of Promises/A+

I don't feel very strongly either way - bluebird (or the type checker) warns me when I pass something I didn't intend (say - a promise) to .then anyway.

[jhusain]

Seems like arguments vis a vis data validation and fail fast were raised in the Promises thread but they still settled on no validation. Under the circumstances I'm inclined to be consistent with Promises. I'm closing this issue in favor of this resolution for now, but I will raise the issue with committee members in the meeting later this month. As far as I'm concerned this isn't a major semantic, and should be able to be changed after we reach Stage 2.

[benjamingr]

@jhusain I don't feel strongly about it - but it does cause a lot of confusion in promises and I recall at least 10 StackOverflow questions where it shot users in the foot.

[ljharb]

fwiw, I feel pretty strongly that passing a non-function should be an error. Happy to wait until the meeting to discuss it tho (and agreed it's a stage 2, not a stage 1, concern)

[jhusain]

After chatting with folks at the committee about this, I'm inclined to agree with @ljharb. I'm leaning towards the following validation logic:

class Observable {
    // snip...
    subscribe(nextOrObserver, error, complete) {
        let next;
        let observer;
        let nextOrObserverType = typeof nextOrObserver;
        if (nextOrObserverType === "object") {
            observer = nextOrObserver;
            if (error != null) {
                throw new TypeError("error is not undefined.");
            }
            if (complete != null) {
                throw new TypeError("complete is not undefined.");
            }

            next = getMethod(nextOrObserver, "next");
            error = getMethod(nextOrObserver, "error");
            complete = getMethod(nextOrObserver, "complete");
        }
        else {
            observer = undefined;
            next = nextOrObserver;
            if (next != null && nextOrObserverType !== "function") {
                throw new TypeError("nextOrObserver is not a function or an object.");
            }

            if (error != null && typeof error !== "function") {
                throw new TypeError("error is not a function.");
            }

            if (complete != null && typeof complete !== "function") {
                throw new TypeError("complete is not a function.");
            }
        }

        return new Subscription(observer, next, error, complete, this._subscriber);
    }
}

Not sure the error messages are correct. Tricky because subscribe is not monomorphic unlike most JS functions. Interested in committee feedback on what the right error messages would be.

[appsforartists]

So the proposal is:

complete and error are optional, but if they have values, ensure those values are functions?

[RangerMauve]

@appsforartists Unless the first argument is an object, then the other two arguments must not be defined.

[benjamingr]

@jhusain fantastic, thanks for the update.