Nit: for validation prefer using internal/validators (e.g. for validating an object you can call validateObject) etc

Can options == null ever happen here given it has a default value of {} ?

function f(o = {}) {
  console.log({ o })
}

f(null)

Outputs

{ o: null }

This is a copy from setTimeout

Ah so it can be null but not undefined - you can check === in that case probably

Ditto regarding validateAbortSignal and internal/validators

this timing logic is incorrect. some examples:

const it = setInterval(100);
it.next(); // should resolve after 100ms
it.next(); // should resolve after 200ms

const it = setInterval(100);
await delay(50);
it.next(); // should resolve after 50ms
it.next(); // should resolve after 150ms

const controller = new AbortController();
const it = setInterval(100, { signal: controller.signal });
it.next(); // below should reject this promise
controller.abort();

This may have to be an option to enable one or the other timing..

Here is an example that brings the idea behind using timing for each .next()

const iterable = setInterval(100);
const iterator = iterable[Symbol.asyncIterator]();
await iterator.next()
await delay(150)
await iterator.next()
await delay(150)

This would be the same as

for await (const unused of setInterval(100)) {
  await delay(150)
}

There is also a difference between the "async iterable" instance (that can be passed directly to for await) vs the iterator creation time, and then the next request to wait for the next interval

Timing given we're hooking onto the next occurring interval from iterator.next being called

setInterval call - 0ms
next call - 0ms
next resolve - 100ms
delay call - 100ms
delay resolve - 250ms
next call - 250ms 
next resolve - 300ms  <- we skipped 1 intervals, 200ms, was 50ms since `next` invoked
delay call - 300ms
delay resolve - 450ms
next call - 450ms
next resolve - 500ms <- we skipped 1 interval, 400ms, was 50ms since `next` invoked

Timing given we have one promise for each interval:

setInterval call - 0ms
next call - 0ms
next resolve - 100ms
delay call - 100ms
delay resolve - 250ms
next call - 250ms
next resolve - 250ms <- Because its now on the 200ms promise, was 0ms since `next` invoked
delay call - 250ms
delay resolve - 400ms
next call - 400ms
next resolve - 400ms <- Because we can never get back now, 300ms promise, was 0ms since `next` invoked

The current timing would be

setInterval call - 0ms
next call - 0ms
next resolve - 100ms
delay call - 100ms
delay resolve - 250ms
next call - 250ms
next resolve - 350ms <- 100ms since `next` invoked
delay call - 350ms
delay resolve - 500ms
next call - 500ms
next resolve - 600ms <- 100ms since `next` invoked

const iterable = setInterval(100);
const iterator = iterable[Symbol.asyncIterator]();
const first = iterator.next()
const second = iterator.next()

The above code seems a bit confusing... we would need to "queue" those resolves then, maybe thats fine?

const defers = []

setInterval(() => {
  const next = defers.shift()
  next.resolve({ done: false, value })
}, after)

/* ... */

next() {
  /* ... */
  
  const deferred = /* ... */
  defers.push(deferred)
  return deferred.promise
},
return() {
   /* ... */
   
   const result = { done: true, value: undefined }
   defers.forEach(deferred => deferred.resolve(result))
   return result
}

I think one, or the other, or both sets of logic will be good, just need to know what one

Im in favour of having:

• Timeouts triggered within next as the default behaviour (keeping consistent behaviour across all use cases)
• A single interval for all iterators as an option
• A single interval for each iterator as an option (as it would be if we implemented like timers: introduce setInterval async iterator #35841 (comment))

If we add the two additional options, this would definitely add to the overall complexity for sure, I would think we would need three completely seperate implementations for this.

I believe next should be asserted to being called only after the previous promise was resolved as well (e.g. deferred not existing in the current implementation), rather than leaving this open to interpretation, else we will most likely need to queue multiple promises to be resolved for each iterator at the same time, rather than one in flight per iterator

A single interval for each iterator as an option (as it would be if we implemented like #35841 (comment))

Isn't that the mental model when someone does setInterval?

The disadvantage of my implementation in that comment is "what happens if the user 'misses' an interval?" The timing is right but the code should probably push the callbacks to an array (instead of replacing a reference) and fulfil them in the interval.

Also cc @ronag wdyt about what we should do with timers -> async iterators conversion in terms of behavior?

Reading the HTML spec for setInterval, it runs the setTimeout steps, with a "repeat" flag that repeats the steps again post task (which for the setInterval, it is a sync task)

https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-setinterval

The setInterval() method must return the value returned by the timer initialization steps, passing them the method's arguments, the object on which the method for which the algorithm is running is implemented (a Window or WorkerGlobalScope object) as the method context, and the repeat flag set to true.

The repeating happens after the related tasks function has been invoked (which in our case is the external code executing, the stuff between next calls) and for any errors to be caught

Run the appropriate set of steps from the following list:

If the first method argument is a Function
Invoke the Function. Use the third and subsequent method arguments (if any) as the arguments for invoking the Function. Use method context proxy as the callback this value. If this throws an exception, catch it, and report the exception.

Then

If the repeat flag is true, then call timer initialization steps again, passing them the same method arguments, the same method context, with the repeat flag still set to true, and with the previous handle set to handler.

The running of the task now is async, we tell the consumer to start the task by resolving the task, and the consumer signifies the task is complete by calling next again (or return)

Aside from the above, we could warn if we skip intervals and leave it at that, but then this seems like inconsistent behaviour, if I had a 30 minute interval going, happened to take 31 minutes for my task, and now have to wait for 29 minutes till the next interval.... leading to the intervals actually being once an hour

Conceptually I would imagine it to be equivalent to adding an await on timer._onTimeout() here:

Ah! As I say that, I see start is set before _onTimeout is invoked 🤔

Reading the HTML spec for setInterval, it runs the setTimeout steps, with a "repeat" flag that repeats the steps again post task (which for the setInterval, it is a sync task)

Well, two things:

• That's just an easy way to specify things and doesn't actually indicate when it runs.
• We don't follow that (the web timers) spec (even remotely) though if we built Node today then we probably would.

What I meant is just "the standard queueing trick" something like:

async function* iteratorSetInterval(ms, options = {}) {
  // validateOptions
  // validateAbortSignal
  let callbacks = [], pending = 0;

  let aborted = options && options.signal && options.signal.aborted;
  if (options && options.signal) {
    options.signal.addEventListener('abort', () => {
      aborted = true;
    });
  }
  
  const timerRef = setInterval(() => {
    if (callbacks.length > 0) callbacks.shift()();
    else pending++;
  }, ms);
  if (options && options.ref === false) {
    timerRef.unref();
  }

  try {
    while (!aborted) {
      yield await new Promise((resolve) => { 
         if (pending > 0) { resolve(); pending--; } ;
         else callbacks.push(resolve);
      });
    }
  } finally {
    clearInterval(timerRef);
  }
}

Async iterators don't need to reject on cancellation or expose the abort error - it's enough to .return on the iterator which would cause whatever is wrapping it (for await, stream etc) to close.

This is definitely a question I hoped to come up in review.

@/jasnell had suggested endOnSignal to be an option to either reject or resolve for this.

cc @jasnell - i think the mental model is that streams and iterators have a concept of "abort" built in unlike promises - so it's better to use it probably :]

I can see it both ways really. An option makes it more complicated, but gives users the flexibility. I'm honestly not sure which should be the default here.

I avoid doing cancellation "the regular JS way" with .return and not with a rejection/error unless we have a very compelling reason not to do the "standard" thing.

Note that if we implement this with an async generator this solution will work "automatically" by putting cleanup in a finally block