update typings to indicate Seq() faux-iterator support

[ShawnTalbert]

To support pre-ES2015 environments where Symbol.iterator does not exist, recognizing
that the Seq() implementation does accept a 'false' Iterable in the form of an object that has a
property named "@@iterator" which returns the Iterator.

Discussion on PR #1589

[conartist6]

Looks pretty straightforward to me.

[leebyron]

I'd rather avoid this change's approach and leverage one where ES5 environments use their own definition of Iterable.

Even if we did go for this approach, this change is too limited, there are well over 100 references to Iterable across the type definitions, not just input to Seq. It doesn't make sense to only change the types for this one location.

See my comment on the PR in concern for thoughts on another approach

[ShawnTalbert]

The Seq() implementation seems to support real Iterable<T> with Symbol and also any old object that has an @@iterable function that returns an iterator (faux). That's a handy feature that actually made my code work in the ES5 environment (yay!).

In general, isn't it best for Seq() typings to actually match its capabilities - i.e. it simultaneously supports Iterable and the faux one.

Your suggestion should work, but it would be lying about Seqs actual interface. Additionally, as a user I'd have to override a set of well known ES types (Iterable and friends) to be a an implementation specific definition.

The helpful declarations you provide in the PR comment are technically specific to ImmutabjeJS. Shouldn't all library-specific declarations be somewhere in immutable.d.ts?

[conartist6]

I was about to say something similar actually. Changing the libdefs would affect the language and all other libraries. It's the kind of thing that would make me swear very loudly if I discovered someone else had done it in a codebase I had to work in. It shouldn't be that hard to update the definition of Iterable across all the immutable.d.ts types. Just define an iterable.d.ts, export your modified definition of Iterable, and import it in immutable.d.ts. You won't have to touch any of the usages individually.

[ShawnTalbert]

Standing in the shoes of a user it would be much appreciated if we didn't conflate the faux iterable and the real one into a single type named Iterable<T> as it would be confusing.

If everywhere Iterable<T> is allowed, so is FauxIterable<T> then is it any harder than a single search-n-replace Iterable<T> with Iterable<T> | FauxIterable<T>?

If a single type name is preferred, could we perhaps use something along the lines of
type IterableLike<T> = Iterable<T> | FauxIterable<T>
then refer to IterableLike<T> (which is immediately clear for the user that it's not the libdef Iterable<T>)?

Thoughts?

[conartist6]

I definitely see your point, but also you're in the minority here. Most users will be in an environment in which they can use iterables, and it isn't going to be clear to them what IterableLike means. I believe Iterable is the correct name because whether or not you have the special symbol, we're still trying to convey no more and no less than the idea of following the iterable protocol (to the best of the environment's abilities). If there's no good way to take the name of a type and expand it to its definition, that's something you should take up with your IDE.

[ShawnTalbert]

Understood. As you say, a good IDE should be able to easily expand IterableLike to Iterator<T> | FauxIterator<T> :) or, just leave those expanded as I show in this pull request.

Doesn't that make it clear that Seq() can take either an Iterator (the libdef definition everyone is familiar with) or a FauxIterator (which is immutablejs specific)?

I would naturally expect Iterator to be the libdef version (especially since it's being referred to globally) and it would stand out to me that FauxIterator is something different and special - maybe I'd need to take a closer look.

Maybe I'm misunderstanding but trouble with the single name Iterator<T> is it would have a meaning different from (and incompatible with) the libdef Iterator<T>? Though, again if the IDE is good it would show me that Iterator<T> is coming from immutable. Indeed, in my environment (where I don't have libdef Iterator predefined) that should work.

Another option is to just remove the current fake iterator support from Seq() and just force everyone to have some form of Symbol.iterator support (native or shim?). That would make me a bit sad, since Seq() elegantly works with my existing code without me introducing any additional dependencies/shims (provided the FauxIterator type is surfaced somewhere).

In any case (recognizing I'm in the minority), anything y'all can do to resolve this would be acceptable and appreciated!

[ShawnTalbert]

After reviewing by babbling above, since a ES5 environment would not have any Iterator<T> defined, then an Iterator<T> defined by Immutable seems fine.

It would also save me from changing my tsconfig.json to include the Iterable<T> libdef (again, fibbing to my project).

[conartist6]

My priorities are 1. Make it works correctly and continues to work correctly for everyone. 2. Make sure it gives pretty errors in common cases. In my mind distributing the union across all the usages is a no-go because it's hard to maintain. Likely future editors will forget to do it and reviewers will fail to notice and then you'll end up with subtle type bugs. If we support this we should support it well, i.e. in a single place where it can have comments and where we can easily pull it out eventually when it's no longer needed.

[ShawnTalbert]

Agreed, if the Immutable declaration for Iterable<T> allowed the '@@iterator' flavor then I think my code would compile and I can then enthusiastically adopt 4.x!

[conartist6]

Sounds great to me! If you're going to implement it that way just remember that you have to make a separate file to convert from a global name to an export, but that's the only thing that needs to go in that file. Here's an example from some recent work I did: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/d2c1186142999c17273b075a8ddf97ea9e830983/types/error-cause/base/Error.d.ts

[leebyron]

The helpful declarations you provide in the PR comment are technically specific to ImmutabjeJS. Shouldn't all library-specific declarations be somewhere in immutable.d.ts?

Not specific to Immutable! The "@@iterator" key was used in the first version of the Iterable protocol introduced by Firefox long ago! Chrome used it as well. It wasn't formalized in the spec and this was before ES6 (and therefore symbols).

To a degree this is Immutable showing its age. It needed to support iteration protocols in both older and modern browsers, and that's why this "@@iterator" support is still here.

In general, isn't it best for Seq() typings to actually match its capabilities - i.e. it simultaneously supports Iterable and the faux one.

Ideally the type definitions should not care that there are two different execution environments with two different ways of defining the iterator property. I think we should discourage actively mixing the two, since not all other libraries have this broad support and that could lead to confusing errors. That's why my initial advice was to define the version of Iterable<T> that's correct for your environment.

Changing the libdefs would affect the language and all other libraries. It's the kind of thing that would make me swear very loudly if I discovered someone else had done it in a codebase I had to work in.

Yeah this is a very good point and one I suspect you'll need to be practical about based on the situation you're in. It's exactly the reason why Immutable doesn't try to define these well defined types itself (it used to pre-v4!). If you have a very large codebase with many libraries that also have a conception of what Iterable is, then likely your only viable way forward is a global polyfill or shim so that all of those libraries work together without frustration. If Immutable.js is the only library you have that depends on Iterable then you should take license to define that type so that you're unblocked.

Just define an iterable.d.ts, export your modified definition of Iterable, and import it in immutable.d.ts. You won't have to touch any of the usages individually.

This could work for you, but remember that Iterable will need to have some global effects. There are many places across the type defs where an Iterable is expected as input where an Array is valid. You'll need to make sure that an ES5-Firefoxesque version of Iterable libdef isn't just locally imported to a copy of Immutable.d.ts but also appropriately augments the Array interface.

[conartist6]

Ah that's great context. I didn't know @@iterator was a de-facto standard in browsers. Lee if I understand what you're saying your point about confusing errors is that people who intended to pass an iterable as Seq(iterable) will be getting a big error detailing all the types and overloads that couldn't be matched, and now there will be an extra one. That's really no big deal for Seq which already has a list of quite a few possible types, but it's a bigger deal for other methods which currently don't have multiple overloads, such as getIn(keyPath: Iterable<unknown>): unknown;. A failure to match this type would now always print as a failure to match either branch of the union.

I agree that that's a pain, but also just for fun I went to check on the implementation of getIn, and in fact it uses coerceKeyPath which can't accept an iterable at all! Hmmmmmmm.

[ShawnTalbert]

The issue here was the difference between the ES spec (which is reflected in the TypeScript bundled lib definition for Iterable<T>) and what Immutable Seq() actually supports. From that context, whether the custom faux iterable implementation chosen by Immutable happens to coincide with the some pre-standard property name in browsers seems immaterial to the problem at hand?

Succinctly, Seq() accepts a specific faux iterator shape but the immutable.d.ts declarations for that function do not reflect that.

Lee, if the decision is yours, would you kindly confirm that Immutable will not be updating the type definitions to include the full range of acceptable inputs to Seq()?

At least with that cemented I can know whether I'll be able to use 4.x, or be forced to stick with 3.8 forever or choose another library entirely.

[conartist6]

I'll say one last thing: immutable is in no small part a legacy library. It does some pretty incredible things given that it also works in some pretty barebones environments. At some point though I think it should transition from being an es3 library to being an es6 library, and the 4.0 release is an ideal point to make that pivot. The API has been feature-stable for a long time, and 4.0 was largely a polish release getting rid of most known flaws in the implementation. What I'm saying is that I don't think we have to try to bury the idiosyncrasies of 4.x, because they're not long term. We want the people who care about beautiful design and integration to adopt 5.x, which of course we haven't even begun writing.

[leebyron]

Lee if I understand what you're saying your point about confusing errors is that people who intended to pass an iterable as Seq(iterable) will be getting a big error detailing all the types and overloads that couldn't be matched, and now there will be an extra one.

Yes this plus my agreement with your previous point about Iterator being the correct type and avoiding that union confusion across all 100+ uses.

From that context, whether the custom faux iterable implementation chosen by Immutable happens to coincide with the some pre-standard property name in browsers seems immaterial to the problem at hand?

The case I was trying to make before is that this is exactly material. It wasn't that Immutable.js wanted some custom faux iterable implementation and happened to choose "@@iterator", that support is there precisely because this was the browser standard before ES6 and Immutable.js wanted to support a broad range of environments. It's not a faux iterable, it's a real iterable standard. It's just not the one defined by ES6, it's the one defined by Firefox years earlier (and adopted by other browsers).

Your case of having an ES5 environment to support is exactly the reason why this behavior exists in Immutable.

Your comment on the other PR
What if Immutable decides to change these faux iterator definitions down the road? I would expect a compile-time error via updated definitions coming from that newer version of the lib.

Very reasonable concern if this was an Immutable.js's definition of an iterator! Since they only exist to support older environments, the only reason we'd ever change them is if we decided to drop support for pre-ES6 browsers, and that would come with a major version change.

The issue here was the difference between the ES spec (which is reflected in the TypeScript bundled lib definition for Iterable) and what Immutable Seq() actually supports.

Totally agree. Runtime behavior is supporting ES5 correctly, so this is a purely a matter of supplying the correct TypeScript types.

Maybe this seems like a subtle difference, but I think it's actually quite important to get right - Seq() (and other functions that operate on iterables) have runtime support for ES5 and ES6 Iterables, but we shouldn't conceptualize these as different things - they're both different runtime implementations of one kind of type. We would not expect ES5 and ES6 iterators to be floating around the same runtime environment, and so we probably should not write type libs that allow that.

Also note that Immutable's typedefs are not specifically using the ES6 Iterable type lib. It just expects that you'll have have a TypeScript environment that has this type defined. Since there is a specification for them in ES6, TypeScript supplies that typelib directly for you to include in your environment via lib.es2015.iterable.d.ts. I suspect if the ES5 spec had formally defined them, then TypeScript would have defined lib.es5.iterable.d.ts for use. Then this issue would be very easily resolved - you would just use import the ES5 typelib, instead of the ES6 typelib, since that would correctly describe your execution environment.

My recommendation stands that since TypeScript does not offer this directly, you can provide one yourself. Luckily it is a really small lib to write.

lib.es5.iterable.d.ts

interface Iterator<T> {
  next(): { done: boolean, value: T };
}

interface Iterable<T> {
  ['@@iterable'](): Iterator<T>;
}

interface IterableIterator<T> extends Iterator<T> {
  ['@@iterable'](): IterableIterator<T>;
}

interface Array<T> {
  ['@@iterable'](): IterableIterator<T>;
}

interface ReadonlyArray<T> {
  ['@@iterable'](): IterableIterator<T>;
}

Lee, if the decision is yours, would you kindly confirm that Immutable will not be updating the type definitions to include the full range of acceptable inputs to Seq()?

Since Immutable.js's type defs expect Iterable to be ambiently defined, it already does describe the full range of acceptable inputs as long as your environment uses one of the two implementations our runtime definition supports (ES6 or ES5).

I don't think it would be the right thing to do to have Immutable.js directly define the ES5 Iterables type. Especially since that would impact the Array and ReadonlyArray ambient types, which would not be correct for those using an ES6 environment.

[leebyron]

While directly including ES5 iterators this in the immutable.d.ts file could have breaking changes to the environment, I could still be reasonable to offer this somewhere easy for people to import?

[conartist6]

@ShawnTalbert Does your environment define Array.prototype.@@iterator?

[ShawnTalbert]

Nay, there is no Array.prototype['@@iterator'] in this ES5 world I'm running.

[conartist6]

Right. So I think that means I'm settled in disagreement with @leebyron. If you want to make a change to all environments, the process exists: you get it put into the spec. Since @@iterator is non-spec, you must assume that it is present in only some environments. Thus to me the only path forward that provides correctness in the type system is to widen the type even if errors get uglier.

[leebyron]

If you want to make a change to all environments, the process exists: you get it put into the spec

Today yes. In 2012... specs were stagnant and browsers just shipped shit and copied each other. My point above is that Iterators existed before ES6, even if not uniformly so (browsers did not used to be uniform) and their implementation is the one Immutable.js is designed to support. As opposed to Immutable.js desiring some non-standard Iterator implementation that had a non-material coincidentally similarity to these older environments. If it weren't for supporting those older environments then we'd probably remove it.

Thus to me the only path forward that provides correctness in the type system is to widen the type even if errors get uglier.

I'm not sure why this is our only path forward. It has significant downsides around type complexity and degraded error messages.

It also doesn't solve the key problem that Iterable is an undefined ambient type. For example if we introduce IterableLike<T> = Iterable<T> | FauxIterable<T> and you tell TS that you're using ES5, then it will still complain that Iterable is not defined.

Perhaps another option to solve this could be done with two steps:

1. Expand any input (fn arg) of type Iterable<T> to Iterable<T> | ReadonlyArray<T> so that we remove the assumption that Arrays are iterable, since that might not be a safe one in many ES5 environments. This should be non-breaking. That would further simplify defining your own Iterable ambient so that it would not require extending other globals types.
2. Introduce immutable.es5.d.ts which imports immutable.d.ts and defines the Iterable global ambient using "@@iterator". This would preserve high quality types for all ES6 users, which is the vast majority, while still offering a type definition file to import for ES5 users. This would still require a little bit of config to ensure the right type file was being imported, but perhaps that's preferable to defining your own Iterator type.

Especially after the first step, then an ES5-Firefoxish type def for Iterator becomes simply:

lib.es5.iterator.d.ts

interface Iterator<T> {
  next(): { done: boolean, value: T };
}

interface Iterable<T> {
  ['@@iterable'](): Iterator<T>;
}

interface IterableIterator<T> extends Iterator<T> {
  ['@@iterable'](): IterableIterator<T>;
}

[ShawnTalbert]

Alas, the runtime environment I'm in is server-side, not a browser and has no de-facto concept of Iterator<T> whatsoever. From that perspective, Iterator<T> is a type from Immutable just as much as Seq is.

In my scenario I have two runtimes to support - an ES5 compatible environment (Rhino) and an ES2019 environment (Graal). Also indirectly, ES6+ browsers). To support this with one build, I've been targeting ES5 which functions under both environments due to good backwards compatibility.

I don't see a way to resolve a single global Iterable<T> that works cleanly across ES5/ES6+ without introducing something external (and even then it would be an approximation for ES5). I totally forgot about the point from your last post about the global dependency on an Iterable<T>; seems insurmountable from ES5 without external help. I tried some things but couldn't come up with any way to represent the concept of type Iterable<T> = Iterable<T> ?? FauxIterable<T>

I'm inclined to perhaps just lock the current version of our library to Immutable < 4 and it would remain compatible in all runtimes. Then do a major version bump to target the newer runtime only and hence Immutable 4+. If my use case is really fringe I hate to waste your time.

Admittedly, the majority of what we do now is in the newer runtime, but there's so much legacy code still running in that old runtime.... It's been nice over the years to be able to upgrade library and use it freely in either environment due to ES5 broad compatibility.

[conartist6]

@ShawnTalbert It sounds like perhaps you should just fork immutable 4 and publish @shawntalbert/immutable or whatever would be an acceptable name. Personally I think forking has gotten a stigma that it really doesn't deserve, so when I need something different and can't get maintainers on board, I just go for it. Obviously it's harder though for libraries where I expect there might be a major amount of work keeping my fork up to date (or serious consequences for failing to do so) but I don't think either is the case here.

[leebyron]

Less costly than a fork would be just a fork of the type defs, since the runtime itself doesn't need to change.

Considering that ES6+ Iterables depend on Symbol, and one of your constraints is that you don't want to introduce types that do not accurately reflect the environment you're operating in, then think it's going to be hard to describe one type for Iterable, even as a union.

Options for you going forward:

1. Define two different TS environments, one for ES5 and one for ES6+.
   • Pro: Will give you accurate types per environment. Code shared across environments will be required to conform to each. Can be contributed back so others using ES5 can share the typedefs.
   • Con: Complicates your setup having multiple TS configs, especially if this is a library - all consumers would need to choose the correct one
2. Define Iterable exclusively with the pre-symbol (ES5 friendly) definition.
   • Pro: Simple lowest common denominator that will definitely work in your ES5 env.
   • Con: Won't provide TS support for any native iterables. May create an issue if your ES2019 environment does in fact include ES6 TS type defs such that Iterable is in conflict.
3. Fork the typedefs and replace the Iterable type with something else
   • Pro: Lets you define the type as you wish. Avoids any collision with environmental types.
   • Con: Won't provide TS support for any native iterables. Requires you to maintain your own typedef.
4. Keep using v3.x
   • Pro: If it's working for you, it will keep working for you
   • Con: Won't provide TS support for any native iterables. v4 fixed many logic and api issues that you forego.