Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dead-claudia opened this issue Aug 16, 2018 · 1 comment · May be fixed by #198
Open

Comments

@dead-claudia
Copy link

dead-claudia commented Aug 16, 2018

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
Copy link
Author

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

@dead-claudia dead-claudia linked a pull request Aug 16, 2018 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant