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

toAsyncIterable: remove Promise.resolve call that's already done by the JS runtime #8913

Merged
merged 1 commit into from
Aug 15, 2018
Merged

toAsyncIterable: remove Promise.resolve call that's already done by the JS runtime #8913

merged 1 commit into from
Aug 15, 2018

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Aug 13, 2018

When await-ing something or yield-ing something from an async generator, one doesn't need to call Promise.resolve to cast it to a Promise. The language runtime and all library functions already to that for us. Examples:

Library functions that accept promises always accept other values, too:

Promise.all( [ 1, 2, new Promise( r => setTimeout( () => r( 3 ), 1000 ) ) ] )

This resolves to [ 1, 2, 3 ] after 1 second. The 1 and 2 values are wrapped with Promise.resolve automatically.

await also accepts both promises and non-promises:

const a = await 1; // `a` is assigned `1` in the next microtask tick
const b = await new Promise( r => setTimeout( () => r( 2 ), 1000 ) ); // waits a second
return a + b; // returns (promise of) `3` from the async function

and finally, async generator can yield non-promise values and yet the resulting iterator will always return promises:

async function* agen() { yield 1; yield 2; }
const it = agen();
it.next().then( x => console.log( 'should be 1:', x.value ) );
it.next().then( x => console.log( 'should be 2:', x.value ) );

Every well-designed API that accepts promises should behave this way. See the W3C guide on writing promise-using specs.

This patch also fixes a bug: if the thing passed to toAsyncIterable is an array of thenables that are not instances of the native Promise, e.g., Bluebird promises or objects from some polyfill, the resulting async iterable will "follow" the values of these thenables:

const iterable = [ thenableThatResolvesTo( 1 ), thenableThatResolvesTo( 2 ) ];
for await ( const i of toAsyncIterable( iterable ) ) {
  console.log( i );
}

Before: console.log prints some objects with a then method
After: console.log prints values 1 and 2

Idea for a followup PR:
The toAsyncIterable function does more redundant work that's already done by the for await loop. For example, for await accepts a synchronous iterable or iterator:

// the array is a synchronous iterable, the loop casts it to async and the loop body
// is executed as three separate microtasks with independent call stacks
for await ( const i of [ 1, 2, 3 ] )

// the iterated thing is a sync iterator, casted to async
function* syncGen() { yield 1; yield 2; yield 3; }
for await ( const i of syncGen() )

Therefore, any array, map, or sync iterator is already a valid async iterable, although it's not an async iterator. What does the toAsyncIterable function want to return? The name suggests that it wants to return async iterable, but the unit tests test for async iterator, which is a subset.

…he JS runtime

When `await`-ing something or `yield`-ing something from an async iterator, one doesn't
need to call `Promise.resolve` to cast it to a Promise. The language runtime and all
library functions already to that for us.
@gziolo gziolo added the [Package] Data /packages/data label Aug 15, 2018
@gziolo gziolo added the [Type] Code Quality Issues or PRs that relate to code quality label Aug 15, 2018
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. Thanks for the thorough / enlightening overview. I particularly enjoyed the guide on promise-using specs.

As a point of reference, in continuation of #8096 there may be a goal to deprecate the asynchronous generators altogether, in favor of simple resolver effects (which can be combined with middlewares like the synchronous generators in #8096).

@@ -104,13 +104,8 @@ export function toAsyncIterable( object ) {
object = [ object ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: The comment preceding the condition reads as a continuation which was previously continued as "...of Promises", now removed. This could be rewritten as a plain "Normalize as iterable"

@aduth aduth merged commit c72e8b7 into WordPress:master Aug 15, 2018
@mtias mtias added this to the 3.6 milestone Aug 16, 2018
@jsnajdr
Copy link
Member Author

jsnajdr commented Aug 21, 2018

I particularly enjoyed the guide on promise-using specs.

Yes, that was a great read for me, too :)

As a point of reference, in continuation of #8096 there may be a goal to deprecate the asynchronous generators altogether, in favor of simple resolver effects (which can be combined with middlewares like the synchronous generators in #8096).

One thing I like about the async generator resolvers is that they force all the actions to be dispatched in a separate event loop tick, because they are yielded as promises.

If a selector could synchronously dispatch an action (in a sync resolver), then it could trigger an infinite loop easily:

selector -> dispatch -> reduce -> listeners -> connect -> selector -> dispatch -> ...

Maybe redux-routine and controls have the same property, I don't know -- I got lost quickly when trying to understand the PR.

@jsnajdr jsnajdr deleted the simplify/to-async-iterable-promise-resolution branch August 21, 2018 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants