Add BuiltinIteratorReturn to IterableIterator and AsyncIterableIterator return types

[Ron Buckton (rbuckton)]

This is a follow-on PR to microsoft/TypeScript#58243 to add BuiltinIteratorReturn to all IterableIterator and AsyncIterableIterator return types.

TODO:

• Update the "typescript" dependency in package.json to a shipping version that supports BuiltinIteratorReturn and the TReturn type parameter for IterableIterator/AsyncIterableIterator
• Update README.md to indicate the minimum TypeScript version

[Kagami Sascha Rosylight (saschanaz)]

So BuiltinIteratorReturn can't be the default value to reduce verbosity?

[Ron Buckton (rbuckton)]

It can't be the default value because it's not the default behavior of any iterator, just the native ones provided by the host. User-defined iterables might not match that behavior, and we don't intend to break those cases unnecessarily.

[Ron Buckton (rbuckton)]

Jake Bailey (@jakebailey) could you review? Also, what is the correct approach to handling the typescript reference in package.json here?

[Ron Buckton (rbuckton)]

I've reverted the package.json/package-lock.json changes, but tests will fail until we at least have a nightly to point to

[Jake Bailey (jakebailey)]

Yeah, I would just wait for a nightly.

If we really, really, really need it, we can run pack this on a PR and then reference the "pr-deploys" one on npm. (The artifact is ephemeral so shouldn't be used.)

[Jake Bailey (jakebailey)]

Seems correct; feel free to use a PR build from pr-deploys if you want to be sure before tomorrow.

[Kagami Sascha Rosylight (saschanaz)]

You might want to update TS dependency to nightly to fix the test. (Edit: #1713 (comment) already mentioned it)

[Jake Bailey (jakebailey)]

Updated it to help things along.

[Jake Bailey (jakebailey)]

Yay, green!

[Kenta Moriuchi (petamoriken)]

IMO, BuiltinAsyncIterator is better name (microsoft/TypeScript#58222 (comment)).

[Ron Buckton (rbuckton)]

IMO, BuiltinAsyncIterator is better name (microsoft/TypeScript#58222 (comment)).

Honestly, I agree. I'll change it.

[Ron Buckton (rbuckton)]

The last commit aligns the generator with microsoft/TypeScript#59388

[Ron Buckton (rbuckton)]

I'll update this to the nightly tomorrow before merging

[Ron Buckton (rbuckton)]

How will this change affect @types/web publishing given that it has a dependency on TS 5.6+ for the BuiltinIterator and BuiltinAsyncIterator types? Would it just be published as @types/web@0.0.152 (or similar) with an entry in the README.md, or do we have some other mechanism of ensuring the correct TS version, such as adding an entry to peerDependencies?

[Jake Bailey (jakebailey)]

I'm not 100% certain; the readme for the @types/web package claims compatibility with even TS 4.5, and in general support for old versions. This PR would change that to only support 5.6, so we would need to wait to publish this package for a while, and then it would break compat.

Does @types/web publishing / usage support typesVersions, maybe?

[Ron Buckton (rbuckton)]

It probably could, by tweaking deploy\createTypesPackages.js slightly to add a typesVersions entry, though we'd have to generate two sets of types: one using IterableIterator for older TS, and one using BuiltinIterator for newer TS.

[Kagami Sascha Rosylight (saschanaz)]

Maybe either bump the minor version or add type AsyncIterableIterator<R> = BuiltinAsyncIterator<R, BuiltinIteratorReturn> in the TS side for better compat? (Because this would also become headache for other definitelytyped packages)

[Ron Buckton (rbuckton)]

To bump semver here I think someone with npm permission will have to manually deploy a new set of packages as 0.1.0 and then the deploy script should automatically make future versions as 0.1.x.

TypeScript-DOM-lib-generator/deploy/createTypesPackages.js

Lines 125 to 131 in 2939d4f

	/** @type {*} */
	constnpmPackage=awaitnpmResponse.json();
	constsemverMarkers=npmPackage["dist-tags"].latest.split(".");
	constbumpedVersion=`${semverMarkers[0]}.${semverMarkers[1]}.${
	Number(semverMarkers[2])+1
	}`;

Is a semver bump necessary if we add "typesVersions" to disambiguate?

[Jake Bailey (jakebailey)]

I don't think we need to bump semver if we are just adding a typesVersions; adding that is not a backwards incompatible change (otherwise we'd be totally dead on DT)

[Jake Bailey (jakebailey)]

Do the baselines also need the 5.5 variants? (I think so?)

[Kagami Sascha Rosylight (saschanaz)]

Oh, TIL that and sounds nice. I guess it's fine in that case.

[Ron Buckton (rbuckton)]

Do the baselines also need the 5.5 variants? (I think so?)

They did, yes. I just pushed up a commit that checks them.

I think this should be ready for one more review pass.

[Jake Bailey (jakebailey)]

Actually, do typesVersions work with lib replacement? I think so, reading pathForLibFile, given it seems to still go through regular module resolution? Would be good to quick double check the PR against 5.5 and 4.9. Wish we tested the list of supported versions here too.

Otherwise, things look correct to me.

[Ron Buckton (rbuckton)]

Actually, do typesVersions work with lib replacement? I think so, reading pathForLibFile, given it seems to still go through regular module resolution? Would be good to quick double check the PR against 5.5 and 4.9. Wish we tested the list of supported versions here too.

Otherwise, things look correct to me.

I'll test that locally and report back shortly.

[Kagami Sascha Rosylight (saschanaz)]

It would be nice to have some CI for that here, but I guess that can be done separately.

[Ron Buckton (rbuckton)]

"typesVersions" works, but the autoImports behavior in createTypesPackages.js is causing problems. I'm surprised autoImports is a thing as it seems like it would conflict with --target ES5 since dom.iterable.generated.d.ts tries to load Symbol but it wouldn't exist.

[Ron Buckton (rbuckton)]

The last commit should fix the autoImport reference path added to the index of each @types package, and I've verified "typesVersions" is working correctly for both 5.5.4 and 5.6.0-dev.

[Kagami Sascha Rosylight (saschanaz)]

The question would be that whether it works in 4.4 or otherwise it will still be a breaking change 🤔

[Ron Buckton (rbuckton)]

I've tested it and it works fine with 4.4 as well. I mentioned 5.5 since that's where the split occurs as a result of the "typesVersions" entry, so 5.5.* and earlier (incl 4.4) will use the version in the ts5.5 directory, while 5.6.0-dev and later will use the version in the root.

[Jake Bailey (jakebailey)]

TS 4.4 is well out of support on DT (and that's what most tools use as their window), so I wouldn't worry about that too much. But good that it works!

[Kagami Sascha Rosylight (saschanaz)]

(Maybe this should also use DT window)

[Kagami Sascha Rosylight (saschanaz)]

Are we actually expecting an error here? This looks like it could potentially silence a real failure 🤔

[Ron Buckton (rbuckton)]

the failures that matter are mismatched files. If either the input or output dir don't exist, then the file mismatch will be reported when we traverse whichever side did exist and find the file on the other side is missing.

This actually catches more failures as we previously only checked what was in baseline and ignored new things in generated. This now enumerates all distinct entries from both so we will report on excess baselines or excess outputs.

[Kagami Sascha Rosylight (saschanaz)]

Yeah, but we call stat before entering directory, so not sure this one matters.

[Ron Buckton (rbuckton)]

We call stat on both, but we still traverse the directory if the stat only succeeded on one side or the other, so the readddir can still potentially fail.

[Kagami Sascha Rosylight (saschanaz)]

I don't understand this, is FooAsyncIterator basically same as BarAsyncIterator, because I don't see a difference between this and ReadableStreamAsyncIterator below?

[Ron Buckton (rbuckton)]

They have independent prototypes on the web, so you could theoretically augment ReadbleStreamAsyncIterator independently via its prototype:

varrs=newReadableStream();varproto=Object.getPrototypeOf(rs[Symbol.asyncIterator]());proto;// [object ReadableStream AsyncIterator]

As with ArrayIterator, MapIterator, SetIterator, etc., we've opted to use interface names that more closely align with the underlying prototype.

[Kagami Sascha Rosylight (saschanaz)]

They have independent prototypes because the underlying iterator algorithms are different, but I wouldn't expect any independent members on them that would require separate declarations, at least not in the foreseeable future, because Web IDL simply doesn't have syntax for that and I don't see why IDL would want that.

Mattias Buelens (@MattiasBuelens) may have some opinion.

[Ron Buckton (rbuckton)]

Yes, but the general principle we've agreed on in microsoft/TypeScript#58243 and in design meeting is to try to give these things a relevant name. BuiltinIterator was too broad and confusing, and IteratorObject<T, BuiltinIteratorReturn, unknown> was too unwieldy and verbose. Since we must give these names, we've opted to give them names that reflect the actual runtime reality.

[Kagami Sascha Rosylight (saschanaz)]

Hmmmm. I guess it would be fine as we don't have too many iterable interfaces here, and also matching with TS principle makes sense. I still worry that this would encourage people do weird things that doesn't fit the spec intention though.

[Ron Buckton (rbuckton)]

Kagami Sascha Rosylight (@saschanaz): any other concerns or feedback before this is merged?

[Kagami Sascha Rosylight (saschanaz)]

Besides what I already said, otherwise I'm good.