Improve from

[david-driscoll]

Description:
I tried to keep the changes down to a minimum here, if we feel like they need to split out into several PR's I'm okay with that. This may add pain for consumers that don't use the es6.lib.d.ts declarations. There is work on the TypeScript to help with this pain, see microsoft/TypeScript#6990 and microsoft/TypeScript#6974. Today we could point them at spec/es5.d.ts as a small shim they can use.

1. Replaces Iterator<T> with Iterable<T>.
2. Introduces Observablesque<T> which uses the symbol.

NOTE: The reason these are grouped is both 1 and 2, are changes that break compilation. We have to shim in SymbolConstructor and Iterable<T>.

Details:
Basically what's going on...

Right now we use Iterator<T> as part of ObservableInput<T> but really the interface we're looking for (in code that is) is Iterable<T>. So this change fixes that.

interface Iterable<T> {
    [Symbol.iterator](): Iterator<T>;
}

Also we declare Symbol.observable but this isn't declared anywhere in the interfaces, so there isn't an easy way to add that to ObservableInput<T> without an interface.

[kwonoj]

what's difference this to what we're currently using in es6-shim type definitions? looks identical.

[david-driscoll]

es6-shim doesn't actually shim in Symbol, so the IteratorShim<T> doesn't have the appropriate [Symbol.iterator]() attached to it.

[kwonoj]

what I meant was Iterator specific. Sorry for confusion.

[david-driscoll]

Rebased.

[benlesh]

Small thing here. [Symbol.observable]() should return a type that is Subscribable<T> AND Observablesque<T> at a minimum. (Similar to how Iterators have a Symbol.iterator on them)

[benlesh]

You know what? Scratch that. Don't worry about it, for the purposes of the internals of our library where this is being used, we don't care.

[benlesh]

LGTM

[david-driscoll]

The last thing I thought about, from a release perspective. Should we copy that es5.d.ts file into the dist outputs? That way if a user comes along with a project that breaks because they don't have Symbol, they can add /// <reference path="./node_modules/rxjs/es5.d.ts" /> to easily fix the problem.

I think at the time being this mainly applies to commonjs builds only I think.

[kwonoj]

@david-driscoll what about publish it into typings and let it be installed as same as other type shims?

[david-driscoll]

I like the idea. Proposed name es6-rxjs-shim as it's specific to what rxjs needs.

I'll also add some notes to https://github.com/ReactiveX/rxjs/blob/master/doc/installation.md to direct users to install the typing if they are compiling their TypeScript project with es5 instead of es6.

[kwonoj]

I'll go through changes once again and check this in later today to tomorrow. type definition shim can be dealt with separate PR, I assume.

[david-driscoll]

Sure thing, as long as the name works I'll start the work getting it into the typings registry in parallel.

[robwormald]

Can we test this against core-js and the like? Anything that introduces types like Symbol to the global namespace is almost guaranteed to cause a huge amount of pain downstream...

[kwonoj]

@david-driscoll , let me hold off check in bit - would you able to clarify @robwormald 's request?

[david-driscoll]

@robwormald does core-js have a like typescript typings? This change doesn't actually shim Symbol anywhere, instead it just allows us to use Symbol in es5 like code, so that our interfaces can accept a spec based observable.

The following just won't work, unless the TypeScript code has Symbol.

export interface Observablesque<T> {
  [Symbol.observable](): Subscribable<T>;
}

The problem is lib.d.ts does not have Symbol defined and lib.es6.d.ts does. Anyone target es6 from TypeScript will be able to consume this interface. The es5.d.ts (name can be changed) is so that someone targeting es5 from TypeScript will be able to still use the library. It is intentionally very slim and only introduces the types that rxjs needs to be able to consume Iterable<T> and Observablesque<T>.

interface Symbol { }
interface SymbolConstructor {
  iterator: symbol;
}
declare var Symbol: SymbolConstructor;
interface Iterable<T> {
    [Symbol.iterator](): Iterator<T>;
}
interface Iterator<T> {
    next(value?: any): IteratorResult<T>;
    return?(value?: any): IteratorResult<T>;
    throw?(e?: any): IteratorResult<T>;
}

[benlesh]

Can we test this against core-js and the like?

Give me all of your PRs, @robwormald

[benlesh]

@david-driscoll @kwonoj I labelled this "LGTM" a while ago... is there still work being done here?

[kwonoj]

Change itself is ready, wanted to ensure and clarify @robwormald 's question. I'm planning to check this in around tomorrow.

[david-driscoll]

Along the lines of the discussion with @kwonoj and @robwormald I've created a typings repo, for es5.d.ts. I've added some content to the readme to explain the motivation behind it.

https://github.com/david-driscoll/rxjs-symbol-typings

If someone wants to move this to ReactiveX org, I'm happy to facilitate the move. Or just make a new repo, clone and push this content up, that's cool too.

Once this gets merged into the typings registry, I can create a follow up PR that removes es5.d.ts and uses the new typings. In addition I'll add a few notes to the getting started docs so that users targeting es5 will know that they need to have this additional typing installed to compile correctly.

[david-driscoll]

rebased

[benlesh]

@david-driscoll looks like there's still a build failure. Might want to dig into that.

[benlesh]

Actually it looks like it has something to do with the merged mocha changes. cc/ @kwonoj

[david-driscoll]

@Blesh Nah, it was my bad, I missed something on rebase... third time is the charm I think

[kwonoj]

Merged with c6ef2e8, thanks @david-driscoll

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.