[core-paging] Use a TS v3.4-friendly iterator protocol

[deyaaeldeen]

Relates to #19394 (fixes it but we do not want to close it after merging)

Updates the next method in the PagedAsyncIterableIterator interface to return a promise of IteratorResult<T> instead of IteratorResult<T, T>. The second parameter (the type of return expressions) will default to any. This is not a breaking change because it changes the type of return expressions from T to the less precise type any.

For context, IteratorResult was updated in v3.6 to take an extra type variable that specifies the type of return expressions. The change in this PR enables older versions of the typescript compiler (<3.6) to compile code that uses core-paging since they understand the IteratorResult<T> type only. This change is correct IMO because I never seen any return expressions in our iterators in the first place so promising the customers of all our iterators that there will always be a returned value of type T is incorrect.

I cannot think of a code that could break because of this change.

This PR also adds a simple sample that I used for testing.

[jeremymeng]

Maybe @HarshaNalluru still remember the details? Setting both to T seems to offer a little help in the IDE: for both done cases we get intellisense for our value

type IteratorResult<T, TReturn = any> =
  | IteratorYieldResult<T>
  | IteratorReturnResult<TReturn>;
interface IteratorYieldResult<TYield> {
  done?: false;
  value: TYield;
}
interface IteratorReturnResult<TReturn> {
  done: true;
  value: TReturn;
}

[deyaaeldeen]

Maybe @HarshaNalluru still remember the details? Setting both to T seems to offer a little help in the IDE: for both done cases we get intellisense for our value

Our implementation of next does not have return expressions so I am not sure what intellisense benefit do we get from setting the returned value type.

[HarshaNalluru]

#10599 (comment) reference

Please go through the PR #10599

Try this sample(from storage-blob) to see how the IntelliSense varies with your change, for the version combinations suggested in the #10599 (comment) reference.

GIST - you'll lose the IntelliSense for "value" if I'm not wrong.

#10599 should have fixed the user issue I guess, but it was deleted in #16732 😔

[deyaaeldeen]

I am not sure how storage implements their next but here is the intellisense experience for core-paging's next implementation:
yielded values (done = false) are typed as TElement

returned values (done = true) are typed as any but since there is no returned values anyway, it should not matter!

[deyaaeldeen]

I am adding these to make sure the type of element is not any. The first line is enough for this purpose, but I had to add the second to trick TS into thinking the variable elementHasTypeAny is "used".

[xirzec]

Maybe put a comment to this effect in the code?

[bterlson]

You could wrap this up into an empty assertNotAny function call maybe?

function assertNotAny<T extends (IsAny<T> extends true ? never : any)>(t: T): void { }

could work, assuming the intent is that the compiler yells at you if it is any for some reason. But I'm not sure this needs to be in the samples?

[xirzec]

yeah, couldn't it be a test?

[deyaaeldeen]

@bterlson nice, I used assertNotAny!

@xirzec Good point! I converted it to tests with descriptive names.

[xirzec]

I agree with the rationale that we aren't going to be setting value when done is true, so this works perfectly.

If we wanted to be more precise, I think you could use IteratorResult<T, undefined> and doing some downlevel TS magic to change the definition under 3.6 to IteratorResult<T>, but I believe the fix in this PR is both sufficient and far simpler.

[xirzec]

Maybe put a comment to this effect in the code?

[witemple-msft]

@HarshaNalluru @deyaaeldeen I agree with Deya's reasoning that because we do not, in practice, return values from generators in core-paging (or in storage, from what I checked), that giving it the same type as the iterator's yield is wrong. We are promising the user that we will produce something we don't actually return.

When it comes to IteratorResult, the value is typed dependently of done. IteratorResult<T, T> says "this iterator produces value: T regardless of the state of done, but at runtime I don't think this is true, so I'm in agreement with the plan to remove the second type argument. Users will still get high-quality intellisense for value, but only in contexts where they've checked if the result is done. If they were getting itellisense for value without checking done, then that was incorrect. (For what it's worth @deyaaeldeen I think this is right regardless of TS3.4 compatibility or anything else, so if it gets us compatibility with an older TS version then that's just extras.)

@deyaaeldeen I'm not sure you get much value from having the sample as opposed to a test, but I do think there is room for a set of universal paging samples so that we don't have to write the same "hey look, you can iterate this thing by page" sample many times.

[witemple-msft]

This is quite clever.

[bterlson]

Love seeing a novel any test 😁 I usually roll with 0 extends (1 & T) ? true : false.

[witemple-msft]

This feels more like a test than a sample.

[deyaaeldeen]

Good point, I converted it to tests and kept only the for-await looping in the sample. Not entirely happy with the definition of the operation itself so any suggestions there are welcome.

[witemple-msft]

IteratorResult<T, undefined>

Promising to return undefined might make the compiler unhappy if you don't explicitly write out return undefined;. This is how it behaves in function definitions, at least. I guess this begs the question of whether IteratorResult<T, void> is a sensible thing to write.

[bterlson]

Left a couple non-essential comments, looks good.

[deyaaeldeen]

Since there is some interest in downleveling IteratorResult in order to provide a better type experience in TS v >= 3.6, I went ahead and added it in sandersn/downlevel-dts#61.