tc39 / proposal-observable

Observables for ECMAScript
https://tc39.github.io/proposal-observable/
3.07k stars 90 forks source link

`Observable.from` iteration functions incorrectly assume their observer parameter is native #197

Open dead-claudia opened 6 years ago

dead-claudia commented 6 years ago

In section 2.2.2, Observable.from iteration functions assume that their observer argument is in fact a native subscription observer, when a subclass constructor could choose to pass it any value, including a primitive. Instead, it should do three things:

  1. In the first step, a TypeError if Type(observer) is not Object.
  2. In step 5.e, use ? ToBoolean(? GetV(observer, "closed")) instead of SubscriptionClosed(subscription) to determine closure status. It should also be determined prior to sending the value.
  3. When invoking observer methods, use ? instead of !, since it's not invariant.

Also, as a couple drive-by issues, the GetIterator call should use iterator, not items, and the last step in the loop should return undefined rather than the iterator result, for consistency.

The fixed body would read like this:

  1. Let iterable be the value of the [[Iterable]] internal slot of F.
  2. Let iteratorMethod be the value of the [[IteratorMethod]] internal slot of F.
  3. Let iterator be ? GetIterator(iterable, iteratorMethod).
  4. Repeat
    1. If ? ToBoolean(? GetV(observer, "closed")) is true, then
      1. Perform ? IteratorClose(iterator, undefined).
      2. Return undefined.
    2. Let next be ? IteratorStep(iterator).
    3. If next is false, then
      1. Perform ? Invoke(observer, "complete", « »).
      2. Return undefined.
    4. Let nextValue be ? IteratorValue(next).
    5. Perform ? Invoke(observer, "next", « nextValue »).

In JS, this would roughly be the following callback:

function makeFromCallback(iterable, iteratorMethod) {
    return observer => {
        const iterator = Call(iteratorMethod, iterable)

        if (!IsObject(iterator)) {
            throw new TypeError(
                "`x[Symbol.iterator]()` must return an object!"
            )
        }

        // Technically wrong, but unobservable:
        // https://github.com/tc39/ecma262/pull/1288
        const nextMethod = GetMethod(iterator, "next")

        while (!observer.closed) {
            const result = Call(nextMethod, iterator)

            if (!IsObject(result)) {
                throw new TypeError(
                    "`iterator.next()` must return an object!"
                )
            }

            if (result.done) {
                observer.complete()
                return
            }

            observer.next(result.value)
        }

        const returnMethod = GetMethod(iterator, "return")

        if (
            returnMethod != null &&
            !IsObject(Call(returnMethod, iterator))
        ) {
            throw new TypeError(
                "`iterator.return()` must return an object!"
            )
        }
    }
}
dead-claudia commented 6 years ago

Oh, and Observable.of suffers a similar problem with erroneous assumptions.