From e74098ed10aa0a4fc6ad6b8d6e354895addd00d1 Mon Sep 17 00:00:00 2001 From: Dominic Farolino Date: Thu, 29 Aug 2024 19:29:21 -0700 Subject: [PATCH] DOM: Fix `Observable#from()` [Symbol.iterator] semantics (1/2) See https://github.com/WICG/observable/pull/160, which specs the `Observable#from()` semantics, matching these tests. See also https://crbug.com/363015168 which describes the ways in which our current implementation of `Observable#from()`'s detection semantics are overbroad. This CL makes the implementation of the "detection semantics" match the desired behavior outlined in that issue, and adds a bunch of tests. R=masonf@chromium.org Bug: 363015168, 40282760 Change-Id: Id6cfdd45c44286b298e107635e4283b018f50aaf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5824955 Commit-Queue: Dominic Farolino Reviewed-by: Mason Freed Cr-Commit-Position: refs/heads/main@{#1349019} --- .../tentative/observable-from.any.js | 277 +++++++++++++++--- 1 file changed, 233 insertions(+), 44 deletions(-) diff --git a/dom/observable/tentative/observable-from.any.js b/dom/observable/tentative/observable-from.any.js index 900362d8c3acee..cc4d3119d2170c 100644 --- a/dom/observable/tentative/observable-from.any.js +++ b/dom/observable/tentative/observable-from.any.js @@ -85,43 +85,124 @@ test(() => { "Subscribing again causes another fresh iteration on an un-exhausted iterable"); }, "from(): Iterable converts to Observable"); -// The result of the @@iterator method of the converted object is called: -// 1. Once on conversion (to test that the value is an iterable). -// 2. Once on subscription, to re-pull the iterator implementation from the -// raw JS object that the Observable owns once synchronous iteration is -// about to begin. +// This test, and the variants below it, test the web-observable side-effects of +// converting an iterable object to an Observable. Specifically, it tracks +// exactly when the %Symbol.iterator% method is *retrieved* from the object, +// invoked, and what its error-throwing side-effects are. +// +// Even more specifically, we assert that the %Symbol.iterator% method is +// retrieved a single time when converting to an Observable, and then again when +// subscribing to the converted Observable. This makes it possible for the +// %Symbol.iterator% method getter to change return values in between conversion +// and subscription. See https://github.com/WICG/observable/issues/127 for +// related discussion. test(() => { - let numTimesSymbolIteratorCalled = 0; - let numTimesNextCalled = 0; + const results = []; const iterable = { - [Symbol.iterator]() { - numTimesSymbolIteratorCalled++; + get [Symbol.iterator]() { + results.push("[Symbol.iterator] method GETTER"); + return function() { + results.push("[Symbol.iterator implementation]"); + return { + get next() { + results.push("next() method GETTER"); + return function() { + results.push("next() implementation"); + return {value: undefined, done: true}; + }; + }, + }; + }; + }, + }; + + const observable = Observable.from(iterable); + assert_array_equals(results, ["[Symbol.iterator] method GETTER"]); + + let thrownError = null; + observable.subscribe(); + assert_array_equals(results, [ + "[Symbol.iterator] method GETTER", + "[Symbol.iterator] method GETTER", + "[Symbol.iterator implementation]", + "next() method GETTER", + "next() implementation" + ]); +}, "from(): [Symbol.iterator] side-effects (one observable)"); + +// This tests that once `Observable.from()` detects a non-null and non-undefined +// `[Symbol.iterator]` property, we've committed to converting as an iterable. +// If the value of that property is not callable, we don't silently move on to +// the next conversion type — we throw a TypeError; +test(() => { + let results = []; + const iterable = { + [Symbol.iterator]: 10, + }; + + let errorThrown = null; + try { + Observable.from(iterable); + } catch(e) { + errorThrown = e; + } + + assert_true(errorThrown instanceof TypeError); + assert_equals(errorThrown.message, + "Failed to execute 'from' on 'Observable': @@iterator must be a " + + "callable."); +}, "from(): [Symbol.iterator] not callable"); + +test(() => { + let results = []; + const customError = new Error("@@iterator override error"); + + const iterable = { + numTimesCalled: 0, + + // The first time this getter is called, it returns a legitimate function + // that, when called, returns an iterator. Every other time it returns an + // error-throwing function that does not return an iterator. + get [Symbol.iterator]() { + this.numTimesCalled++; + results.push("[Symbol.iterator] method GETTER"); + + if (this.numTimesCalled === 1) { + return this.validIteratorImplementation; + } else { + return this.errorThrowingIteratorImplementation; + } + }, + + validIteratorImplementation: function() { + results.push("[Symbol.iterator implementation]"); return { - next() { - numTimesNextCalled++; - return {value: undefined, done: true}; + get next() { + results.push("next() method GETTER"); + return function() { + results.push("next() implementation"); + return {value: undefined, done: true}; + } } }; - } + }, + errorThrowingIteratorImplementation: function() { + results.push("Error-throwing [Symbol.iterator] implementation"); + throw customError; + }, }; const observable = Observable.from(iterable); - - assert_equals(numTimesSymbolIteratorCalled, 1, - "Observable.from(iterable) invokes the @@iterator method getter once"); - assert_equals(numTimesNextCalled, 0, - "Iterator next() is not called until subscription"); + assert_array_equals(results, [ + "[Symbol.iterator] method GETTER", + ]); // Override iterable's `[Symbol.iterator]` protocol with an error-throwing // function. We assert that on subscription, this method (the new `@@iterator` // implementation), is called because only the raw JS object gets stored in // the Observable that results in conversion. This raw value must get // re-converted to an iterable once iteration is about to start. - const customError = new Error('@@iterator override error'); - iterable[Symbol.iterator] = () => { - throw customError; - }; let thrownError = null; observable.subscribe({ @@ -130,56 +211,101 @@ test(() => { assert_equals(thrownError, customError, "Error thrown from next() is passed to the error() handler"); - - assert_equals(numTimesSymbolIteratorCalled, 1, - "Subscription re-invokes @@iterator method, which now is a different " + - "method that does *not* increment our assertion value"); - assert_equals(numTimesNextCalled, 0, "Iterator next() is never called"); -}, "from(): [Symbol.iterator] side-effects (one observable)"); + assert_array_equals(results, [ + // Old: + "[Symbol.iterator] method GETTER", + // New: + "[Symbol.iterator] method GETTER", + "Error-throwing [Symbol.iterator] implementation" + ]); +}, "from(): [Symbol.iterator] is not cached"); // Similar to the above test, but with more Observables! test(() => { + const results = []; let numTimesSymbolIteratorCalled = 0; let numTimesNextCalled = 0; const iterable = { - [Symbol.iterator]() { - numTimesSymbolIteratorCalled++; + get [Symbol.iterator]() { + results.push("[Symbol.iterator] method GETTER"); + return this.internalIteratorImplementation; + }, + set [Symbol.iterator](func) { + this.internalIteratorImplementation = func; + }, + + internalIteratorImplementation: function() { + results.push("[Symbol.iterator] implementation"); return { - next() { - numTimesNextCalled++; - return {value: undefined, done: true}; - } + get next() { + results.push("next() method GETTER"); + return function() { + results.push("next() implementation"); + return {value: undefined, done: true}; + }; + }, }; - } + }, }; const obs1 = Observable.from(iterable); const obs2 = Observable.from(iterable); const obs3 = Observable.from(iterable); const obs4 = Observable.from(obs3); - - assert_equals(numTimesSymbolIteratorCalled, 3, "Observable.from(iterable) invokes the iterator method getter once"); - assert_equals(numTimesNextCalled, 0, "Iterator next() is not called until subscription"); + assert_equals(obs3, obs4); + + assert_array_equals(results, [ + "[Symbol.iterator] method GETTER", + "[Symbol.iterator] method GETTER", + "[Symbol.iterator] method GETTER", + ]); + + obs1.subscribe(); + assert_array_equals(results, [ + // Old: + "[Symbol.iterator] method GETTER", + "[Symbol.iterator] method GETTER", + "[Symbol.iterator] method GETTER", + // New: + "[Symbol.iterator] method GETTER", + "[Symbol.iterator] implementation", + "next() method GETTER", + "next() implementation", + ]); iterable[Symbol.iterator] = () => { + results.push("Error-throwing [Symbol.iterator] implementation"); throw new Error('Symbol.iterator override error'); }; let errorCount = 0; const observer = {error: e => errorCount++}; - obs1.subscribe(observer); obs2.subscribe(observer); obs3.subscribe(observer); obs4.subscribe(observer); - assert_equals(errorCount, 4, + assert_equals(errorCount, 3, "Error-throwing `@@iterator` implementation is called once per " + "subscription"); - assert_equals(numTimesSymbolIteratorCalled, 3, - "Subscription re-invokes the iterator method getter once"); - assert_equals(numTimesNextCalled, 0, "Iterator next() is never called"); + assert_array_equals(results, [ + // Old: + "[Symbol.iterator] method GETTER", + "[Symbol.iterator] method GETTER", + "[Symbol.iterator] method GETTER", + "[Symbol.iterator] method GETTER", + "[Symbol.iterator] implementation", + "next() method GETTER", + "next() implementation", + // New: + "[Symbol.iterator] method GETTER", + "Error-throwing [Symbol.iterator] implementation", + "[Symbol.iterator] method GETTER", + "Error-throwing [Symbol.iterator] implementation", + "[Symbol.iterator] method GETTER", + "Error-throwing [Symbol.iterator] implementation", + ]); }, "from(): [Symbol.iterator] side-effects (many observables)"); test(() => { @@ -221,7 +347,8 @@ promise_test(async () => { await promise; - assert_array_equals(results, ["value", "complete()"], "Observable emits and completes after Promise resolves"); + assert_array_equals(results, ["value", "complete()"], + "Observable emits and completes after Promise resolves"); }, "from(): Converts Promise to Observable"); promise_test(async t => { @@ -352,3 +479,65 @@ test(() => { assert_array_equals(results, ["from @@iterator", "complete"]); }, "from(): Promise that implements @@iterator protocol gets converted as " + "an iterable, not Promise"); + +// When the [Symbol.iterator] method on a given object is undefined, we don't +// try to convert the object to an Observable via the iterable protocol. The +// Observable specification *also* does the same thing if the [Symbol.iterator] +// method is *null*. That is, in that case we also skip the conversion via +// iterable protocol, and continue to try and convert the object as another type +// (in this case, a Promise). +promise_test(async () => { + const promise = new Promise(resolve => resolve('from Promise')); + assert_equals(promise[Symbol.iterator], undefined); + promise[Symbol.iterator] = null; + assert_equals(promise[Symbol.iterator], null); + + const value = await new Promise(resolve => { + Observable.from(promise).subscribe(value => resolve(value)); + }); + + assert_equals(value, 'from Promise'); +}, "from(): Promise whose [Symbol.iterator] returns null converts as Promise"); + +// This is a more sensitive test, which asserts that even just trying to reach +// for the [Symbol.iterator] method on an object whose *getter* for the +// [Symbol.iterator] method throws an error, results in `Observable#from()` +// rethrowing that error. +test(() => { + const error = new Error('thrown from @@iterator getter'); + const obj = { + get [Symbol.iterator]() { + throw error; + } + } + + try { + Observable.from(obj); + assert_unreached("from() conversion throws"); + } catch(e) { + assert_equals(e, error); + } +}, "from(): Rethrows the error when Converting an object whose @@iterator " + + "method *getter* throws an error"); + +test(() => { + const obj = {}; + // Non-undefined & non-null values of the `@@iterator` property are not + // allowed. Specifically they fail the the `IsCallable()` test, which fails + // Observable conversion. + obj[Symbol.iterator] = 10; + + try { + Observable.from(obj); + assert_unreached("from() conversion throws"); + } catch(e) { + assert_true(e instanceof TypeError); + assert_equals(e.message, + "Failed to execute 'from' on 'Observable': @@iterator must be a callable."); + } +}, "from(): Throws 'callable' error when @@iterator property is a " + + "non-callable primitive"); + +// TODO(dom@chromium.org): Add another test like the above, but for +// `[Symbol.asyncIterator] = null` falling back to `[Symbol.iterator]` +// conversion.