Iterator helpers throw the error too late

[benjamingr]

Currently iterator helpers throw type errors on first iteration - they should likely do so synchronously.

For example:

Readable.from([1]).map(1); // returns a stream, for...awaiting it will throw the error

Instead I think we should throw synchronously, which is what I believe the spec says.

A fix would be to take the code in operators.js that does validations that is in an async generator and wrap it so that it does those validations in a function called before the async generator.

So instead of:

async function* map(...) {
  validateFoo(...);
}

We'd do:

function map(...) { // not async to throw synchronously
  validateFoo(...);
  return async function*() {

  }();
}

The test at test-stream-map would similarly need to be updated from rejecting asynchronously on iteration to throwing synchronously. I've opened an issue in the iterator helper proposal to be sure.

That's my understanding here: https://tc39.es/proposal-iterator-helpers/#sec-asynciteratorprototype.map

[targos]

If you want to open an issue without using a template, you can go to https://github.com/nodejs/node/issues/new

[benjamingr]

@targos TIL, thanks!

[benjamingr]

I'm opening an issue rather than a fix since I think this issue is a good place for people other than myself and ronag to get involved with the iterator-helpers initiative :)

[benjamingr]

Confirmed the behavior should be to throw synchronously with the proposal - so this is a bug that should be fixed.

[ljharb]

Is the intention for stream methods to match the iterator helpers interface?

[benjamingr]

@ljharb the intention to add most iterator helper proposal methods to the node streams interface.

I intend Node to pass the tc39 tests as much as possible (read: all of them) and would strongly prefer not to move these methods out of "experimental" until the proposal gets to (at least) stage 3.

There are some divergences that exist (for example - tc39/proposal-iterator-helpers#162 which is why I asked for future-compatibility there).

In addition - if/when the spec changes I believe Node.js should change its implementation to align.

[benjamingr]

(This is a good area to be involved in by the way!)

[iMoses]

@benjamingr I'll happily take this, does seem like a great starting point to get involved :)

[benjamingr]

@iMoses great, let me know if you need help getting started - the sooner we land this the better since these APIs are shipping :)