Skip to content

Commit e74098e

Browse files
domfarolinochromium-wpt-export-bot
domfarolino
authored and
chromium-wpt-export-bot
committed
DOM: Fix Observable#from() [Symbol.iterator] semantics (1/2)
See WICG/observable#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. [email protected] Bug: 363015168, 40282760 Change-Id: Id6cfdd45c44286b298e107635e4283b018f50aaf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5824955 Commit-Queue: Dominic Farolino <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1349019}
1 parent f4430cf commit e74098e

File tree

1 file changed

+233
-44
lines changed

1 file changed

+233
-44
lines changed

‎dom/observable/tentative/observable-from.any.js‎

Lines changed: 233 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -85,43 +85,124 @@ test(() => {
8585
"Subscribing again causes another fresh iteration on an un-exhausted iterable");
8686
}, "from(): Iterable converts to Observable");
8787

88-
// The result of the @@iterator method of the converted object is called:
89-
// 1. Once on conversion (to test that the value is an iterable).
90-
// 2. Once on subscription, to re-pull the iterator implementation from the
91-
// raw JS object that the Observable owns once synchronous iteration is
92-
// about to begin.
88+
// This test, and the variants below it, test the web-observable side-effects of
89+
// converting an iterable object to an Observable. Specifically, it tracks
90+
// exactly when the %Symbol.iterator% method is *retrieved* from the object,
91+
// invoked, and what its error-throwing side-effects are.
92+
//
93+
// Even more specifically, we assert that the %Symbol.iterator% method is
94+
// retrieved a single time when converting to an Observable, and then again when
95+
// subscribing to the converted Observable. This makes it possible for the
96+
// %Symbol.iterator% method getter to change return values in between conversion
97+
// and subscription. See https://github.com/WICG/observable/issues/127 for
98+
// related discussion.
9399
test(() => {
94-
let numTimesSymbolIteratorCalled = 0;
95-
let numTimesNextCalled = 0;
100+
const results = [];
96101

97102
const iterable = {
98-
[Symbol.iterator]() {
99-
numTimesSymbolIteratorCalled++;
103+
get [Symbol.iterator]() {
104+
results.push("[Symbol.iterator] method GETTER");
105+
return function() {
106+
results.push("[Symbol.iterator implementation]");
107+
return {
108+
get next() {
109+
results.push("next() method GETTER");
110+
return function() {
111+
results.push("next() implementation");
112+
return {value: undefined, done: true};
113+
};
114+
},
115+
};
116+
};
117+
},
118+
};
119+
120+
const observable = Observable.from(iterable);
121+
assert_array_equals(results, ["[Symbol.iterator] method GETTER"]);
122+
123+
let thrownError = null;
124+
observable.subscribe();
125+
assert_array_equals(results, [
126+
"[Symbol.iterator] method GETTER",
127+
"[Symbol.iterator] method GETTER",
128+
"[Symbol.iterator implementation]",
129+
"next() method GETTER",
130+
"next() implementation"
131+
]);
132+
}, "from(): [Symbol.iterator] side-effects (one observable)");
133+
134+
// This tests that once `Observable.from()` detects a non-null and non-undefined
135+
// `[Symbol.iterator]` property, we've committed to converting as an iterable.
136+
// If the value of that property is not callable, we don't silently move on to
137+
// the next conversion type — we throw a TypeError;
138+
test(() => {
139+
let results = [];
140+
const iterable = {
141+
[Symbol.iterator]: 10,
142+
};
143+
144+
let errorThrown = null;
145+
try {
146+
Observable.from(iterable);
147+
} catch(e) {
148+
errorThrown = e;
149+
}
150+
151+
assert_true(errorThrown instanceof TypeError);
152+
assert_equals(errorThrown.message,
153+
"Failed to execute 'from' on 'Observable': @@iterator must be a " +
154+
"callable.");
155+
}, "from(): [Symbol.iterator] not callable");
156+
157+
test(() => {
158+
let results = [];
159+
const customError = new Error("@@iterator override error");
160+
161+
const iterable = {
162+
numTimesCalled: 0,
163+
164+
// The first time this getter is called, it returns a legitimate function
165+
// that, when called, returns an iterator. Every other time it returns an
166+
// error-throwing function that does not return an iterator.
167+
get [Symbol.iterator]() {
168+
this.numTimesCalled++;
169+
results.push("[Symbol.iterator] method GETTER");
170+
171+
if (this.numTimesCalled === 1) {
172+
return this.validIteratorImplementation;
173+
} else {
174+
return this.errorThrowingIteratorImplementation;
175+
}
176+
},
177+
178+
validIteratorImplementation: function() {
179+
results.push("[Symbol.iterator implementation]");
100180
return {
101-
next() {
102-
numTimesNextCalled++;
103-
return {value: undefined, done: true};
181+
get next() {
182+
results.push("next() method GETTER");
183+
return function() {
184+
results.push("next() implementation");
185+
return {value: undefined, done: true};
186+
}
104187
}
105188
};
106-
}
189+
},
190+
errorThrowingIteratorImplementation: function() {
191+
results.push("Error-throwing [Symbol.iterator] implementation");
192+
throw customError;
193+
},
107194
};
108195

109196
const observable = Observable.from(iterable);
110-
111-
assert_equals(numTimesSymbolIteratorCalled, 1,
112-
"Observable.from(iterable) invokes the @@iterator method getter once");
113-
assert_equals(numTimesNextCalled, 0,
114-
"Iterator next() is not called until subscription");
197+
assert_array_equals(results, [
198+
"[Symbol.iterator] method GETTER",
199+
]);
115200

116201
// Override iterable's `[Symbol.iterator]` protocol with an error-throwing
117202
// function. We assert that on subscription, this method (the new `@@iterator`
118203
// implementation), is called because only the raw JS object gets stored in
119204
// the Observable that results in conversion. This raw value must get
120205
// re-converted to an iterable once iteration is about to start.
121-
const customError = new Error('@@iterator override error');
122-
iterable[Symbol.iterator] = () => {
123-
throw customError;
124-
};
125206

126207
let thrownError = null;
127208
observable.subscribe({
@@ -130,56 +211,101 @@ test(() => {
130211

131212
assert_equals(thrownError, customError,
132213
"Error thrown from next() is passed to the error() handler");
133-
134-
assert_equals(numTimesSymbolIteratorCalled, 1,
135-
"Subscription re-invokes @@iterator method, which now is a different " +
136-
"method that does *not* increment our assertion value");
137-
assert_equals(numTimesNextCalled, 0, "Iterator next() is never called");
138-
}, "from(): [Symbol.iterator] side-effects (one observable)");
214+
assert_array_equals(results, [
215+
// Old:
216+
"[Symbol.iterator] method GETTER",
217+
// New:
218+
"[Symbol.iterator] method GETTER",
219+
"Error-throwing [Symbol.iterator] implementation"
220+
]);
221+
}, "from(): [Symbol.iterator] is not cached");
139222

140223
// Similar to the above test, but with more Observables!
141224
test(() => {
225+
const results = [];
142226
let numTimesSymbolIteratorCalled = 0;
143227
let numTimesNextCalled = 0;
144228

145229
const iterable = {
146-
[Symbol.iterator]() {
147-
numTimesSymbolIteratorCalled++;
230+
get [Symbol.iterator]() {
231+
results.push("[Symbol.iterator] method GETTER");
232+
return this.internalIteratorImplementation;
233+
},
234+
set [Symbol.iterator](func) {
235+
this.internalIteratorImplementation = func;
236+
},
237+
238+
internalIteratorImplementation: function() {
239+
results.push("[Symbol.iterator] implementation");
148240
return {
149-
next() {
150-
numTimesNextCalled++;
151-
return {value: undefined, done: true};
152-
}
241+
get next() {
242+
results.push("next() method GETTER");
243+
return function() {
244+
results.push("next() implementation");
245+
return {value: undefined, done: true};
246+
};
247+
},
153248
};
154-
}
249+
},
155250
};
156251

157252
const obs1 = Observable.from(iterable);
158253
const obs2 = Observable.from(iterable);
159254
const obs3 = Observable.from(iterable);
160255
const obs4 = Observable.from(obs3);
161-
162-
assert_equals(numTimesSymbolIteratorCalled, 3, "Observable.from(iterable) invokes the iterator method getter once");
163-
assert_equals(numTimesNextCalled, 0, "Iterator next() is not called until subscription");
256+
assert_equals(obs3, obs4);
257+
258+
assert_array_equals(results, [
259+
"[Symbol.iterator] method GETTER",
260+
"[Symbol.iterator] method GETTER",
261+
"[Symbol.iterator] method GETTER",
262+
]);
263+
264+
obs1.subscribe();
265+
assert_array_equals(results, [
266+
// Old:
267+
"[Symbol.iterator] method GETTER",
268+
"[Symbol.iterator] method GETTER",
269+
"[Symbol.iterator] method GETTER",
270+
// New:
271+
"[Symbol.iterator] method GETTER",
272+
"[Symbol.iterator] implementation",
273+
"next() method GETTER",
274+
"next() implementation",
275+
]);
164276

165277
iterable[Symbol.iterator] = () => {
278+
results.push("Error-throwing [Symbol.iterator] implementation");
166279
throw new Error('Symbol.iterator override error');
167280
};
168281

169282
let errorCount = 0;
170283

171284
const observer = {error: e => errorCount++};
172-
obs1.subscribe(observer);
173285
obs2.subscribe(observer);
174286
obs3.subscribe(observer);
175287
obs4.subscribe(observer);
176-
assert_equals(errorCount, 4,
288+
assert_equals(errorCount, 3,
177289
"Error-throwing `@@iterator` implementation is called once per " +
178290
"subscription");
179291

180-
assert_equals(numTimesSymbolIteratorCalled, 3,
181-
"Subscription re-invokes the iterator method getter once");
182-
assert_equals(numTimesNextCalled, 0, "Iterator next() is never called");
292+
assert_array_equals(results, [
293+
// Old:
294+
"[Symbol.iterator] method GETTER",
295+
"[Symbol.iterator] method GETTER",
296+
"[Symbol.iterator] method GETTER",
297+
"[Symbol.iterator] method GETTER",
298+
"[Symbol.iterator] implementation",
299+
"next() method GETTER",
300+
"next() implementation",
301+
// New:
302+
"[Symbol.iterator] method GETTER",
303+
"Error-throwing [Symbol.iterator] implementation",
304+
"[Symbol.iterator] method GETTER",
305+
"Error-throwing [Symbol.iterator] implementation",
306+
"[Symbol.iterator] method GETTER",
307+
"Error-throwing [Symbol.iterator] implementation",
308+
]);
183309
}, "from(): [Symbol.iterator] side-effects (many observables)");
184310

185311
test(() => {
@@ -221,7 +347,8 @@ promise_test(async () => {
221347

222348
await promise;
223349

224-
assert_array_equals(results, ["value", "complete()"], "Observable emits and completes after Promise resolves");
350+
assert_array_equals(results, ["value", "complete()"],
351+
"Observable emits and completes after Promise resolves");
225352
}, "from(): Converts Promise to Observable");
226353

227354
promise_test(async t => {
@@ -352,3 +479,65 @@ test(() => {
352479
assert_array_equals(results, ["from @@iterator", "complete"]);
353480
}, "from(): Promise that implements @@iterator protocol gets converted as " +
354481
"an iterable, not Promise");
482+
483+
// When the [Symbol.iterator] method on a given object is undefined, we don't
484+
// try to convert the object to an Observable via the iterable protocol. The
485+
// Observable specification *also* does the same thing if the [Symbol.iterator]
486+
// method is *null*. That is, in that case we also skip the conversion via
487+
// iterable protocol, and continue to try and convert the object as another type
488+
// (in this case, a Promise).
489+
promise_test(async () => {
490+
const promise = new Promise(resolve => resolve('from Promise'));
491+
assert_equals(promise[Symbol.iterator], undefined);
492+
promise[Symbol.iterator] = null;
493+
assert_equals(promise[Symbol.iterator], null);
494+
495+
const value = await new Promise(resolve => {
496+
Observable.from(promise).subscribe(value => resolve(value));
497+
});
498+
499+
assert_equals(value, 'from Promise');
500+
}, "from(): Promise whose [Symbol.iterator] returns null converts as Promise");
501+
502+
// This is a more sensitive test, which asserts that even just trying to reach
503+
// for the [Symbol.iterator] method on an object whose *getter* for the
504+
// [Symbol.iterator] method throws an error, results in `Observable#from()`
505+
// rethrowing that error.
506+
test(() => {
507+
const error = new Error('thrown from @@iterator getter');
508+
const obj = {
509+
get [Symbol.iterator]() {
510+
throw error;
511+
}
512+
}
513+
514+
try {
515+
Observable.from(obj);
516+
assert_unreached("from() conversion throws");
517+
} catch(e) {
518+
assert_equals(e, error);
519+
}
520+
}, "from(): Rethrows the error when Converting an object whose @@iterator " +
521+
"method *getter* throws an error");
522+
523+
test(() => {
524+
const obj = {};
525+
// Non-undefined & non-null values of the `@@iterator` property are not
526+
// allowed. Specifically they fail the the `IsCallable()` test, which fails
527+
// Observable conversion.
528+
obj[Symbol.iterator] = 10;
529+
530+
try {
531+
Observable.from(obj);
532+
assert_unreached("from() conversion throws");
533+
} catch(e) {
534+
assert_true(e instanceof TypeError);
535+
assert_equals(e.message,
536+
"Failed to execute 'from' on 'Observable': @@iterator must be a callable.");
537+
}
538+
}, "from(): Throws 'callable' error when @@iterator property is a " +
539+
"non-callable primitive");
540+
541+
// TODO([email protected]): Add another test like the above, but for
542+
// `[Symbol.asyncIterator] = null` falling back to `[Symbol.iterator]`
543+
// conversion.

0 commit comments

Comments
 (0)