-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Publishing to a lifted Subject is type-unsafe #1234
Comments
@masaeedu when Subject's |
@trxcllnt Could you direct me to the code where this is done? |
@masaeedu everywhere in the Subject class that references |
Yes, I realize that, but my point was that you never (as far as I know), actually call |
Note that I'm not talking about situations where you |
@masaeedu yes that does happen. I don't use TypeScript so the compiler doesn't complain at me, but I do |
I think the complaint here is really that TypeScript doesn't support higher-kinded typing when it really should if its target is JavaScript. We could fix this if we were building our .d.ts files manually I suppose. Perhaps @david-driscoll knows a better way? |
@trxcllnt So let's look at this scenario: import {Subject} from 'rxjs/rx';
// Setup subject and a subscription
let numbers = new Subject<number>();
let rounded = numbers.map(Math.round);
rounded.subscribe(n => console.log(n));
// lift into something that produces, (and accepts?) strings
let strings = numbers.map(n => n.toString());
strings.subscribe(s => console.log(s));
// Push in some values
numbers.next(1.5);
strings.next("Hello"); As a user, especially a TypeScript user, it is confusing to see this result in:
This is not how |
@Blesh I don't think higher kinded types would make this better. In the example above, the subscription to
the existing module somehow needs to be recompiled with the awareness that downstream code is passing in strings. There is no way to represent this in a type system. I guess this goes back to #541; maybe I'm just trying to project my static typing biases onto a project that just isn't suited for it 😄 |
@Blesh without writing the types manually there would be no good way to do this. As for the behavior, it seems fairly confusing. That is specifically why would a subscription to a subject be bound to another subject much deeper in the chain. I can understand it being useful for WebSockets, but it could also cause additional confusion later on. I put @masaeedu's changes on requirebin and was confused, basically it's just forwarding all |
var socket = Rx.Observable.websocket(socketURL);
var doesSomeStuff = socket.flatMap(...).filter(...);
someFunction(doesSomeStuff);
function someFunction(socket) {
socket.subscribe((x) => {
console.log('hello!' + x);
socket.next(x + ' acknowledged');
});
}
const obs = subject.asObservable(); |
|
@trxcllnt Wouldn't this work?
Why do you need to post back to the mapped, filtered etc. subject? It's not like the values you're nexting in the subscribed handler are being passed through a "reversed" chain of inverse operators; they're just being naively propagated from one subject to the next until they get back to the original subject. |
@trxcllnt Note that the above scenario would be enabled by simply removing the |
@masaeedu yes, but you don't have to keep a reference to the original socket. if that chain can be maintained through the Subject implementation, I haven't heard a good why we wouldn't. Infinite loops don't occur for the same reason they don't occur today in RxJS 4 when you |
Infinite loop is easy, not with a WebSocket because it's async nature.
|
@trxcllnt Well the reason is that it breaks type safety for us picky TypeScript users 😄. I don't think it's so bad to have to keep a reference to a websocket. Regarding the infinite loop, wouldn't posting to the websocket result in the subscribed handler being called, which would post a value to the websocket, which would call the handler and so forth? I'm just asking to check if there's any special message routing machinery that would be broken by the change I'm proposing. |
@david-driscoll that's always been possible with Subjects (minus the Subject out of the
Agree to disagree.
Perhaps in a naive implementation. My use-case is in classes that extend Subject, and I need my sub-types available so I can keep calling my custom operators. If we have to change the type signature of Subject from |
Could you elaborate on why it's worse to have a direct reference to the websocket than to have the chain of
This is what I was trying to check. Would it not be possible to change whatever the implementation does to avoid the infinite loop encountered by a naive implementation, so that it still works without the
This would be a useful change for |
@trxcllnt But this is the use case we're talking about, no? The situation where |
Because the WebSocket may or may not actually exist if, for example, it is lazily created and destroyed based on ref-counting.
This isn't a linked-list of references to destination. The intermediate Subjects share the same reference to lift<T, R>(operator: Operator<T, R>): Observable<T> {
const subject = new Subject(this, this.destination || this); // <---
subject.operator = operator;
return <any>subject;
} I can sympathize with the argument that |
Ah yes, that's a good point regarding |
Could you provide an example where using a direct reference to the original subject behaves differently from using the last result in an operator chain? |
@masaeedu there is no difference between the two aside from convenience. |
Ok, so just to be clear, the reason the direct reference is worse is because it is inconvenient, not because of a difference in ref counting, correct? As a compromise, maybe you could implement the |
@masaeedu the advantage of implementing two-way communication in the Subject base class is that any Subject implementation or subclass is two-way compatible. I suppose we could remove the |
@trxcllnt What would the fixed type signature look like? The cognitive problem for me right now is that a |
Perhaps we could make a |
A fixed type signature, is any method that With doing any major work on this yet we could do something like.... interface CoreOperators<T, O extends Observable<T>> {
map(selector: any): O;
}
...
export class Observable<T> implements CoreOperators<T, Observable<T>> {...}
export class Subject<T> implements CoreOperators<T, Subject<T>> {...} Problem is |
@david-driscoll Observable is just a wrapper for the |
@david-driscoll IMO that's making the problem worse, because it's now exposing the API that causes type unsafety to TypeScript developers, who care about type safety the most. Let's say the signature was Right now they get a compile error on the last line, which I think mitigates the problem somewhat. |
Summarizing the suggestions in this thread:
Do any of these sound like a reasonable plan of action @trxcllnt, @Blesh? |
@masaeedu @david-driscoll does this look any better? I got it to where the compiler isn't yelling at me anymore: class Subject<T, T_Back> extends Observable<T> implements Observer<T_Back>, Subscription {
protected source: Observable<any>;
protected destination: Observer<T_Back>;
protected observers: Observer<T>[];
constructor(destination?: Observer<T_Back>, source?: Observable<any>) {
super();
this.source = source;
this.destination = destination;
}
lift<R>(operator: Operator<T, R>): Subject<R, T_Back> {
const subject = new Subject<R, T_Back>(this.destination || this, this);
subject.operator = operator;
return subject;
}
next(value: T | T_Back): void {
if (this.destination) {
return this.destination.next(<T_Back> value);
}
const observers: Observer<T>[] = this.observers.concat();
let index = -1;
const len = observers.length;
while (++index < len) {
observers[index].next(<T> value);
}
}
error(error: any): void {
if (this.destination) {
return this.destination.error(error);
}
const observers: Observer<T>[] = this.observers;
this.observers = null;
let index = -1;
const len = observers.length;
while (++index < len) {
observers[index].error(error);
}
}
complete(): void {
if (this.destination) {
return this.destination.complete();
}
const observers: Observer<T>[] = this.observers;
this.observers = null;
let index = -1;
const len = observers.length;
while (++index < len) {
observers[index].complete();
}
}
unsubscribe(): void {}
add(subscription: Subscription| Function | void): void {}
remove(subscription: Subscription): void {}
}
const numbers = new Subject<number, number>();
const strings = <Subject<string, number>> numbers.map<string>((num) => {
return num.toString(2);
});
numbers.subscribe((num: number) {
console.log(num);
});
strings.subscribe((str: string) {
console.log(str);
});
numbers.next(5);
// prints
// 5
// "101"
strings.next(10);
// prints
// 10
// "1010" |
I know I'm slow to respond, but that looks better assuming it compiles, it would be a breaking change for making subjects though (from the views of TS user anyway), so probably not very desirable. In TypeScript it will probably still return Observable, but that should be okay. For this convince case it's probably simpler for the consumer to just typecast to |
Closing this issue as stale. |
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. |
The
lift
method inSubject.ts
looks like this:The
T
parameter there is redundant, and causes a loss of type safety. Thelift
method on aSubject<Foo>
should accept anOperator<Foo, ?>
, not anOperator<?, ?>
. A similar change forObservable.lift
andOperator.call
has been proposed in #1214.A few other issues to consider:
Observable<T>
. The newSubject
it is returning has as its operator anOperator<T, R>
, and will therefore behave as anObservable<R>
Observer
functionality ofsubject
is hidden and cannot be accessed, since the return type isObservable<T>
Subject
is set tothis.destination
. Assuming you took aSubject<number>
, lifted it through.map(n => n.toString())
, and triednext
ing strings to the resultingSubject<string>
, the destination of the original subject, which expects numbers, would be handed stringsOverall, I don't think this override is necessary. The behavior for
lift
provided in theObservable
class (which is to return a new observable, set its source tothis
, and attach an operator), is the same as what is implemented here. The additional behavior of wrapping in aSubject
that posts tothis.destination
is type-unsafe and unused (at least within the codebase).The text was updated successfully, but these errors were encountered: