Skip to content

Should it await next.value when not passing a mapper function? #19

Closed
@nicolo-ribaudo

Description

@nicolo-ribaudo
Member

Consider this async iterable:

const it = {
  [Symbol.asyncIterator]() {
    return {
      async next() {
        if (i > 2) return { done: true };
        i++;
        return { value: Promise.resolve(i), done: false }
      }
    }
  }
}

Unless I'm reading the spec wrong, await Array.fromAsync(it) returns [Promise.resolve(1), Promise.resolve(2), Promise.resolve(3)] while await Array.fromAsync(it, x => x) returns [1, 2, 3].

Currently, when using Array.from, x => x is the "default mapper function" (even if it's not specified as such): Array.from(something) is always equivalent to Array.from(something, x => x). However, there is no "default function" that can be passed to Array.fromAsync.

If we changed Array.fromAsync(something) to await the next.value values, await Array.fromAsync(it) would always return the same result as await Array.fromAsync(it, x => x). In practice, it means making it behave like

const arr = [];
for await (const nextValue of it) arr.push(await nextValue);

Activity

nicolo-ribaudo

nicolo-ribaudo commented on Jan 3, 2022

@nicolo-ribaudo
MemberAuthor

I just noticed that the readme says (emphasis is mine)

mapfn is a mapping callback, which is called on each value yielded from the input – the result of which is awaited then added to the array. Unlike Array.from, mapfn may be an async function.) By default, this is essentially an identity function.

ljharb

ljharb commented on Jan 3, 2022

@ljharb
Member

If the default is identity, then explicitly passing identity must of course produce the same values. My intuition is that if the explicit identity awaits, then the implicit one also should.

js-choi

js-choi commented on Jan 3, 2022

@js-choi
Collaborator

Yes, good catch; I agree. The value of each iteration should be awaited when no mapping function is given. This is a spec bug, and I will fix it soon.

added a commit that references this issue on Jan 5, 2022
a46172e
zloirock

zloirock commented on Jan 5, 2022

@zloirock
Contributor

Iteration helpers do not await in similar cases.

In for-await-of, nextValue is a promise, required explicit awaiting.

I think that it should be agreed upon between both proposals.

zloirock

zloirock commented on Jan 5, 2022

@zloirock
Contributor
added a commit that references this issue on Jan 5, 2022
cf50e8c
added a commit that references this issue on Jan 5, 2022
56fcd21
zloirock

zloirock commented on Jan 5, 2022

@zloirock
Contributor

@js-choi it's better to leave it open until aligning it with the iterator helpers proposal.

reopened this on Jan 5, 2022

4 remaining items

brad4d

brad4d commented on Jan 29, 2022

@brad4d

I'm hoping that this bit of code clarifies what this issue is about.

// Draft polyfill
Array.asyncFrom = async function(iterable, mapFn = x => x, thisArg = null) {
  const result = [];
  for await (const value of iterable) {
    // NOTE: If the iterable produces a Promise, `value` will be its resolved
    // value, because the for-await syntax awaits it.
    // See: https://github.com/tc39/proposal-async-iteration/issues/15
    const mapFnResult = mapFn.call(thisArg, value);

    // This issue (https://github.com/tc39/proposal-array-from-async/issues/19)
    // is about deciding whether the `await` should appear here or not.
    result.push(await mapFnResult);
  }
  return result;
}

Assuming I haven't gotten this wrong, I think we should have the await shown above, because that is the most consistent with the behavior of for-await, which awaits the individual elements it is iterating over.

js-choi

js-choi commented on Jan 29, 2022

@js-choi
Collaborator

Some representatives at the plenary a few days ago stated that they wanted to avoid double awaiting and optimize for the more-common case of omitting the mapping function, so we will need to revisit this issue.

At the very least, we will need to do a thorough comparison of every choice we have, before presenting to plenary again.

RubyTunaley

RubyTunaley commented on Mar 28, 2022

@RubyTunaley

I mean the way I see it there's only really 5 options:

Only await mapped values if they are promise-like

This requires a check for a then property with type 'function' on each returned mapped value, but if omitting mapfn is like 95% of cases then this might be fine, since when undefined is passed the runtime can branch into a fast path that doesn't do that check.

The type check is there to alleviate this concern:

For instance, when we don't supply amapping call back and we have an AC and DC link input. We can, we can not await anything. Which we can't because we want some sort of identity callback equivalency.

- JSC, TC39 Meeting Notes, January 2022

This solution seems to be the one the committee was leaning towards, although I'm unsure whether they would be happy with the behaviour of a mapfn like x => Math.random() < 0.5 ? x : Promise.resolve(x).

Have null and undefined mean different things

  • If the mapfn parameter is undefined, execute with no mapping and without double-await.
  • Else:
    • If the mapfn parameter is null, set mapfn to x => x.
    • Execute with mapping and with double-await.

Array.from currently doesn't accept null for mapfn so it would be a good idea to modify it so that it does too, although in that case it can have the same behaviour as undefined.

Split the function

The function could be split into Array.fromAsync and something like Array.fromAsyncMap, but that's inconsistent with how Array.from works and it can't be fixed without either breaking the web or breaking the parameter symmetry between from and fromAsync.

Drop mapping

Just get developers to write (await Array.fromAsync(blah)).map(x => x) (or with iterator-helpers await Array.fromAsync(blah.map(x => x))).

Are there any stats available for how often Array.from's mapfn parameter is even used?

Accept either double-awaiting everything or having undefined behave differently from x => x.

The committee has made its opinion clear on double-awaiting everything, but having undefined behave differently is unintuitive (which is why the null option exists).

bakkot

bakkot commented on Jul 6, 2022

@bakkot
Contributor

await Array.fromAsync(it) returns [Promise.resolve(1), Promise.resolve(2), Promise.resolve(3)] while await Array.fromAsync(it, x => x) returns [1, 2, 3]

This seems like the right answer to me. And Array.fromAsync(it, x => { console.log(x); return 0; }) would print three Promises and return [0, 0, 0].

I am OK with having undefined behave differently from x => x, because x => x isn't acting as the identity function here - its result is being awaited. (One way to look at this is that the second argument is an async function [edit: rather, to be precise, it's a function composed with await], even if you happen to write it as a sync function, and there isn't an identity async function.)

bakkot

bakkot commented on Jul 6, 2022

@bakkot
Contributor

Note that for await does not await these values:

let i = 0;
let it = {
  [Symbol.asyncIterator]() {
    return {
      async next() {
        if (i > 2) return { done: true };
        i++;
        return { value: Promise.resolve(i), done: false }
      }
    }
  }
};

(async () => {
  for await (let v of it) {
    console.log(v); // prints a promise
  }
})();

I really think we should match that behavior when not passing the second argument.

js-choi

js-choi commented on Jul 10, 2022

@js-choi
Collaborator

I think @bakkot gives some persuasive points, especially that the mapping function is actually essentially an async function, so it wouldn’t make sense for its identity to be x => x.

My priorities, in order, have always been:

  1. Array.fromAsync(i) must be equivalent to Array.fromAsync(i, undefined) and Array.fromAsync(i, null). (For optional parameters, nullish arguments should be functionally equivalent to omitting the arguments. This is how every function in the core language is designed, and I believe it is also an explicit best practice in Web APIs.)

  2. Array.fromAsync(i) must be equivalent to for await (const v of i). (The default case of fromAsync must match intuitions about for await (of), just like how from matches intuitions about for (of).)

  3. Array.fromAsync(i) should be equivalent to AsyncIterator.from(i).toArray().

  4. Array.fromAsync(i, f) should be equivalent to AsyncIterator.from(i).map(f).toArray().

  5. Array.fromAsync(i, f) should conceptually but not strictly be equivalent to Array.from(i, f).

I lost sight of the second priority when I wrote #20.

Bakkot points out that the default mapping function of Array.fromAsync does not have to be x => x, and omitting the mapping function does not have to be equivalent to specifying x => x or some other function.

Therefore, I plan to revert my changes in #20. The default behavior without a mapping function will be to not await values yielded by async iterators. When a mapping function is given, the inputs supplied to the mapping function will be the values yielded by the input async iterator without awaiting; only the results of the mapping function will be awaited. This behavior should match AsyncIterator.prototype.toArray.

function createIt () {
  return {
    [Symbol.asyncIterator]() {
      let i = 1;
      return {
        async next() {
          if (i > 2) {
            return { done: true };
          }
          i++;
          return { value: Promise.resolve(i), done: false }
        },
      };
    },
  };
}

Without any mapping function:

result = [];
for await (const x of createIt()) {
  console.log(x);
  result.push(x);
}
// result is [ Promise.resolve(1), Promise.resolve(2), Promise.resolve(3) ].

result = await Array.fromAsync(createIt());
// result is [ Promise.resolve(1), Promise.resolve(2), Promise.resolve(3) ].

With mapping function x => x:

result = await Array.fromAsync(createIt(), x => x);
// result is [ 1, 2, 3 ].

result = await AsyncIterator.from(createIt())
  .map(x => x)
  .toArray();
// result is [ 1, 2, 3 ].

With mapping function x => (console.log(x), x):

result = await Array.fromAsync(createIt(), x =>
  (console.log(x), x));
// Prints three promises.
// result is [ 1, 2, 3 ].

result = await AsyncIterator.from(createIt())
  .map(x => (console.log(x), x))
  .toArray();
// Prints three promises.
// result is [ 1, 2, 3 ].

With mapping function async x => (console.log(await x), await x)):

result = await Array.fromAsync(createIt(), async x =>
  (console.log(await x), await x));
// Prints 1, 2, then 3.
// result is [ 1, 2, 3 ].

result = await AsyncIterator.from(createIt())
  .map(async x => (console.log(await x), await x))
  .toArray();
// Prints 1, 2, then 3.
// result is [ 1, 2, 3 ].
added a commit that references this issue on Jul 11, 2022

Revert #20 and do not await async inputs’ values without mapping ƒ

74384a4
locked as resolved and limited conversation to collaborators on Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    questionFurther information is requested

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Participants

      @js-choi@ljharb@brad4d@bakkot@zloirock

      Issue actions

        Should it await `next.value` when not passing a mapper function? · Issue #19 · tc39/proposal-array-from-async