Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolves #1723 - added typings for store.observable method. #1725

Closed
wants to merge 1 commit into from
Closed

Resolves #1723 - added typings for store.observable method. #1725

wants to merge 1 commit into from

Conversation

babeal
Copy link

@babeal babeal commented May 13, 2016

I went with the generic Observable and Observer names even though the interfaces are only partially implemented. You might want more specific names like Subscribable and NextObserver or no names at all with the typings inline for that method. Let me know.

@gaearon
Copy link
Contributor

gaearon commented May 13, 2016

cc @Blesh — do you TypeScript by any chance?

@benlesh
Copy link
Contributor

benlesh commented May 14, 2016

I do. It looks fine except the Symbol.observable method itself. That's tricky because that static property didn't exist on Symbol in TypeScript yet. So you have to add it by defining it on Symbol, I believe.

@babeal
Copy link
Author

babeal commented May 15, 2016

@Blesh - So is the typing for "Symbol.observable" something that the Redux repository should provide? I would have thought that I would have found that typing extension in the symbol-observable repository but it seems that it only exports the basic symbol. Let me know if I am understanding this correctly $$observable ~= Symbol.observable?

I've walked through the symbol-observable, redux-observable and redux repositories trying to understand what it's for. I also took the typescript example to see what typings it would produce from the code. It seems that there isn't an explicit typing provided for symbol property access which is a little weird to me. Do you have any insight? Is it just not supported?

##file

"use strict";
const getClassNameSymbol = Symbol();

class C {
    [getClassNameSymbol](){
       return "C";
    }
}

let c = new C();
let className = c[getClassNameSymbol]();
console.log(className);

##typings

declare const getClassNameSymbol: symbol;
declare class C {
}
declare let c: C;
declare let className: any;

@benlesh
Copy link
Contributor

benlesh commented May 15, 2016

Honestly, it's probably not terribly important to every day use to provide type information for this right off. TypeScript's handling of interfaces with symbols in them is still pretty primitive

@babeal
Copy link
Author

babeal commented May 20, 2016

@gaearon - is it cleared?

@gaearon
Copy link
Contributor

gaearon commented May 23, 2016

@babeal I’m confused: how does it work without Symbol?

@sbabeal
Copy link

sbabeal commented May 24, 2016

It's not nearly as pretty but I am able to still use the observable method instead of the subscribe method. Since I am using RxJs, I can't just use the observable as is, so I still need to create one but it is cleaner than the subscribe function. Also, I am assuming that the symbol is used for finding the property that exposes the observable, but even if it is missing from the typings, it could be added later. I guess I read that from @Blesh 's comment that it wasn't important to type the symbol part for the time being but maybe he meant the whole thing. I figured it would be helpful as the first step; I'll let you guys decide :-).

Observable.create(observer => {
    store.observable(observer);
});

@benlesh
Copy link
Contributor

benlesh commented May 25, 2016

Let's have this more professionally vetted by TS typings experts...

I summon @mhegazy and @david-driscoll!!!

/me throws pokemon ball thing

@benlesh
Copy link
Contributor

benlesh commented May 25, 2016

@mhegazy and @david-driscoll... if you're just arriving, the biggest issue I see here is how to support TS typings for an interface like Observableable below:

interface Subscription() {
  unsubscribe(): void
}

interface IObserver<T> {
  next(value: T): void;
  error(err: any): void;
  complete(value: any): void;
}

interface IObservable<T> {
  subscribe(observer: IObserver<T>): ISubscription
}

interface Observableable<T> {
  [Symbol.observable]() : IObservable<T>
}

The problem is that Symbol.observable isn't a thing in TS.

@mhegazy
Copy link

mhegazy commented May 25, 2016

the biggest issue I see here is how to support TS typings for an interface like Observableable below:

The complete support for symbol-named properties on interfaces is not there yet. this is tracked by microsoft/TypeScript#5579.

The part that is supported is "well-known symbols". so if you make observable a well known one (just like you defined it above on the global Symbol object, and always access it on the global Symbol object), then you should be good.

for instance:

// Add the declaration on the global Symbol constructor
interface SymbolConstructor {
    readonly observable: symbol;
}

// Use it to define a property
interface Observableable<T> {
  [Symbol.observable]() : IObservable<T>
}

// Use it to access a property
var o: Observableable<string>;
o[Symbol.observable]().subscribe // works

but aliasing it, importing it, assigning to another value does not work:

import {observable} from "SymbolDefinitions";
o[observable]().subscribe // does not work

@babeal
Copy link
Author

babeal commented May 27, 2016

I think I was too excited to see an observable method on the store to realize that the observable method isn't exported as a function named observable. The only way to access this method is through the Symbol.observable. Some things aren't as easy as they seem.

I gave it a go, and the suggestions for declaring observable on the SymbolConstructor work when added to the global declaration. But there isn't a way around the typescript-definition-tester that's executing against ES5. So both the errors, "Cannot find name Symbol and Module augmentation cannot introduce new names in the top level scope. will prevent moving forward. So, I'm not sure what you want to do here. Nevertheless, what I really wanted was a true Rx5 observable for state, so I am just going to stick with my wrapper and move forward. @gaearon if you want to hold off go ahead and close this. The typings in the pull request aren't correct anyway.

@gaearon
Copy link
Contributor

gaearon commented Jun 14, 2016

Let’s revisit when there is first class support for this pattern in TS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants