Typing fixes #1214

masaeedu · 2016-01-18T23:14:52Z

The Observable.prototype.lift and Operator.prototype.call have unbound generic type parameters at the function level. Current signatures are:

// src/Operator.ts
export class Operator<T, R> {
  call<T, R>(subscriber: Subscriber<R>): Subscriber<T> {
    return new Subscriber<T>(subscriber);
  }
}

// src/Observable.ts
...
lift<T, R>(operator: Operator<T, R>): Observable<R> {
  const observable = new Observable<R>();
  observable.source = this;
  observable.operator = operator;
  return observable;
}
...

The T type parameter in each of those functions shouldn't be there, since T in each of those definitions needs to be bound to the T argument of the containing type.

The other changes are removing the redundant type argument in a few operators. The change also revealed that MergeMapToOperator doesn't implement the Operator interface correctly:

export class MergeMapToOperator<T, R, R2> implements Operator<Observable<T>, R2> {
  ...
  call(observer: Subscriber<R2>): Subscriber<T> {
    return new MergeMapToSubscriber(observer, this.ish, this.resultSelector, this.concurrent);
  }
}

An Operator<Observable<T>, R2> needs to implement a call method that returns Subscriber<Observable<T>>, not Subscriber<T>. Since I'm not familiar with what the mergeMapTo operator is supposed to do (the doc comment is missing), I couldn't change MergeMapToSubscriber to implement Subscriber<Observable<T>>. I've fixed it by replacing T with any for now, but this may bear further investigation.

luisgabriel · 2016-01-18T23:40:43Z

src/Operator.ts

@@ -1,7 +1,7 @@
 import {Subscriber} from './Subscriber';

 export class Operator<T, R> {
-  call<T, R>(subscriber: Subscriber<R>): Subscriber<T> {
+  call<R>(subscriber: Subscriber<R>): Subscriber<T> {


In this case, the type R of call does not have any relation with R of Operator?

Good catch; that's incorrect. R should be bound to the type's R as well. I think fixing this will greatly expand the scope of operators that need to be touched however. I'll try it out.

luisgabriel · 2016-01-18T23:45:50Z

Just minor details regarding the commit messages: you should not capitalize the first letter, and you should use present tense. More info on CONTRIBUTING.md#subject

masaeedu · 2016-01-19T00:12:53Z

@luisgabriel Sorry, missed that bit in the contribution guidelines. Looks like fixing that second parameter has indeed broken a bunch of other operators that can only be fixed by introducing any as a quick hack (as I did with MergeMapToOperator) or figuring out how the types flow through the implementation.

I've actually already worked out the latter approach in a different branch (I'm planning on retracing the changes in small commits), but it involves lots of code being touched throughout the codebase. Would it be better for now to any any operators that incorrectly implement Operator?

luisgabriel · 2016-01-19T00:32:44Z

I'm not sure what would be the best solution here. Could you point out this branch you said? I think I can have a better idea of the problem size.

It's also worth to wait for some inputs from @kwonoj and @david-driscoll in this matter.

masaeedu · 2016-01-19T00:42:37Z

Sure; take a look at the typingFixesAllAtOnce branch. Unfortunately there's a number of semi-related changes in there that make things a bit hairy. They include deduplication of the function signatures by adding an interface to each operator file and various changes in the implementation to make types flow correctly. Those changes aren't necessarily in a finished state.

You could probably get a better idea of which operators would need to be fixed solely as part of this bugfix by:

Cloning the typingFixes branch
Removing the R parameter from Operator
Trying to build

kwonoj · 2016-01-19T00:45:49Z

I was trying same (get rid of <R>) at the same moment, and agree it requires amount of changes to make correct type inferences.

My vote in here is make changes correctly at once, avoiding one additional steps of introducing any. Maybe PR can be separated per effort required, but for me removing redundant generic <R> and updating signature is atomic, should be one changes.

masaeedu · 2016-01-19T00:57:36Z

I agree that would be the ideal approach @kwonoj, but the problem is that making the types flow correctly inside the implementations requires a lot of domain knowledge and isn't a simple chore type task. For this reason it would be easier if it could be fixed in separate PRs on a case by case basis.

In the typingFixesAllAtOnce branch I linked, I fixed implementations for whichever operators included doc comments, but there were still a few where I couldn't find or understand the expected behavior and had to use any.

kwonoj · 2016-01-19T01:03:01Z

As I already mentioned, I agree that requires sufficient time to make corresponding changes. I'm not object to make separate PR for those effort, while this PR leaving <R> for now if those requires noticeable input for making changes. Still, I'm bit opposing to introduce interim any for incremental changes. PR #1077 would be reference for this kind of cases - even though it's amount of changes if it's logical atomic chunk, it'd be better to be changes at once.

masaeedu · 2016-01-19T01:54:41Z

Ok, I see your point. :) So in terms of direction forward, here's a couple of problems that pop up after removing the <R> which I don't know how to address:

webSocket.ts: Mismatch in type of operator property
lift<T, R> problem exists in Subject as well. Removing T results in similar type mismatch of operator property

There are also compile errors in the following files, however I already have tentative fixes for each of these in typingFixesAllAtOnce which need to be checked for correctness:

defaultIfEmpty.ts
every.ts
isEmpty.ts
pairwise.ts
race-support.ts
reduce-support.ts
scan.ts
skipwhile.ts
withLatestFrom.ts

I will clean these up and add them as individual commits to this branch. Once all of these are addressed I think we will be rid of the extra <T, R>.

masaeedu · 2016-01-20T00:27:24Z

@kwonoj Ok, so I've cleaned up all the ones I know how to fix.The only remaining failures at this point are the webSocket and Subject failures. If you could take a look and indicate out how those can be fixed that would be much appreciated.

kwonoj · 2016-01-20T00:40:50Z

Wow, this is huge ;) appreciate for effort. Let's start look into, I'll add some inputs if I have some questions around. /cc @david-driscoll also if he could give some helps.

luisgabriel · 2016-01-20T00:43:13Z

src/operator/pairwise.ts

-export function pairwise<T>(): Observable<T> {
-  return this.lift(new PairwiseOperator());
+export function pairwise<T>(): Observable<[T, T]> {
+  let _this: Observable<T> = this;


Why _this?

+whereever possible, let's use const instead.

@luisgabriel The idea is that using this gives you any, and then all type checking from that point on is disabled. Using only _this ensures your type signatures are compatible (i.e. lifting through the operator actually produces the output type of the function). We can also do this using casts, but I arrived at this pattern over the course of a number of changes across other operators, and repeatedly casting to Observable<T> becomes tedious (and of course if you forget, you don't get any errors from the compiler).

Got it. Thanks for the explanation.

I agree with you. This pattern leads to a better readability over casting. I'd prefer to name it self instead of _this, but this is a minor detail.

I would go with self probably because the compiler will user _this internally for fat arrow methods.

I'm bit on fence between casting vs using explicit typed reference. So it'll be basically

const self: Observable<T> = this; self.doxxx();

vs.

((Observable<T>)this).doXXX()...

correct? While I like prior snippet's clarity of type to self, also makes feel does it necessarily have to hold another reference which is repeated over every operator. (course I also does agree @masaeedu about casting, just speaking out loud to hear some opinions)

@kwonoj That's correct. David raised the performance aspect of this as well. Just so we can make this explicit, the concern is that the instance of Observable would be disposed by the GC if not for the _this/self reference, so adding this alias will result in memory pressure/a full out leak. Is this a fair assessment?

I will try to set up a test in Chrome memory mode to see how the alias affects frequency of GC and memory usage.

If I was a betting man, my bet would be that this has no significant impact, since the locally scoped self reference is orphaned as soon as the operator function returns (provided we don't start passing it into closures). So the lifetime of the object is dictated only by how long external references to the observable persist.

However, as mentioned earlier, I'll do a test in Chrome to backup my handwaving :)

Testing this properly is blocked by #1228, so I'll just change these back to this for now.

on the self vs _this vs source...

So far source has been an idiom in this library. While I realize that these all end up as members of a class, the reality is that it's only so we can compose the operators left to right. These operators rely on almost no state at all from the instance, and under the right conditions (say the passage of the "pipe operator" |>), they could just be functions.

Let's stick with source. There isn't a compelling reason to change it.

david-driscoll · 2016-01-20T00:57:36Z

The point about Subject.lift makes me thing that you should either just cast this to <any> and not worry about the underlying details, or Subject needs an additional generic type parameter.

I'm inclined to say cast it to any, because most consumers will be making subjects, and having two type arguments would be confusing.

kwonoj · 2016-01-20T00:58:21Z

src/Operator.ts

@@ -1,7 +1,11 @@
 import {Subscriber} from './Subscriber';

-export class Operator<T, R> {
-  call<T, R>(subscriber: Subscriber<R>): Subscriber<T> {
+export interface Operator<T, R> {


do we expect export interface and implementation both under same name? would you able to elaborate use case for interface is required with same name of implementation?

@kwonoj I think this was as a result of some difference in parameter variance when implementing an interface instead of "implementing" a class. I can't recall exactly why I had this, so will remove.

masaeedu · 2016-01-20T01:13:29Z

@kwonoj Would you agree just casting to <any> is fine here?

kwonoj · 2016-01-20T01:15:21Z

Would you agree just casting to is fine here?

: It'd be appreciate if you can point 'here', since comment is not pointing specific code and due to size of change, it's bit hard to find right context. ;)

masaeedu · 2016-01-20T01:21:28Z

Sorry, I was referring to David's comment there, should have made it clearer. If you look at my last commit, you'll see the problem. The lift method on Subject is supposed to produce (presumably), an Observable<R>. However, the first line in lift tries to make a Subject<R> using nothing but the instance of Subject<T> itself. Fixing in a type safe way would involve adding another type parameter to Subject, which would be somewhat inconvenient and would probably involve a lot of changes wherever it is referenced.

masaeedu · 2016-01-22T01:34:36Z

@david-driscoll, @kwonoj Would it make sense to just remove the lift operator from Subject and inherit the behavior from Observable? I don't really see that it makes sense to try and feed the transformed values produced by the lifted observable into the same destination.

E.g. you have a Subject<number>, which consists of an Observable of numbers and an Observer of numbers. You lift this through .map(n => n.toString()) to produce another ?<string>. Would this be a Subject<string> or an Observable<string>?

Right now the code is trying to produce a new Subject, with a few quirks:

It sets the destination of the new Subject to its own destination, but this will result in our Observer<number> getting fed strings if anyone actually tries to push values to the new Subject
The source of the new Subject<string> is set to the original Subject<number>, which is fine, but you already get this behavior from the Observable.prototype.lift operator this method is overriding
The newly created subject is returned as an Observable, which indicates to me that the return value isn't expected to be used as a Subject anymore

Removing the override of lift in Subject doesn't affect tests at all. It would be great if someone who thoroughly understands this machinery could provide input.

luisgabriel · 2016-01-22T01:40:06Z

@masaeedu I think @Blesh and @trxcllnt can provide some input in this matter.

kwonoj · 2016-01-22T02:00:56Z

As an immediate thought I think it's fine to creating chain of subject via lift, but interesting point. Interface already limits to return observable only, limiting created object to behave as subject.. @masaeedu What about creating those as separate issue? I don't think this PR's scope can contain those changes anyway. (also this PR's already has bit of size for its own discussion)

masaeedu · 2016-01-22T02:10:59Z

@kwonoj The reason I'm bringing this up here is because we need to make lift compile again for Subject after removing the redundant generics. If you want I can revert all changes to Subject (including removal of the redundant generics), and address both problems in a separate issue.

kwonoj · 2016-01-22T02:16:56Z

I think we can choose between

leave this PR as is until get conclusion on subject behavior
revert current changes on subject to make this PR progress while discuss (your proposal)

I'll leave up to you which way to proceed.

masaeedu · 2016-01-22T02:47:56Z

Ok, opened #1234 and will nuke the Subject changes.

benlesh · 2016-01-22T20:47:49Z

src/operator/every.ts

+
+  let _this: Observable<T> = this;
+  if (_this._isScalar) {
+    let scalar: ScalarObservable<T> = <any>_this;


When this transpiles, won't it have two additional lines where we're setting _this = this and then scalar = _this? I'm all for good typing for the exposed API. Our internals, though, I just care that they work.

It's true that there's no direct benefit to end users for this kind of change, but since I was doing typing fixes in this PR, I also tried to prevent mistakes in typings in the future. The isEmpty operator you commented on earlier is a good example of this. You're trying to take an Observable<T> and produce an Observable<boolean>, but you're using an Operator<boolean, boolean> to do it, which the type system would disqualify. If you cast this to (<Observable<T>>this), the compiler complains about how the operator has the wrong type, which has the knock on effect of forcing you to fix your types all the way down to IsEmptySubscriber.

In and of itself this has no effect on the end user, but in many cases the user facing type signature is wrong simply because of a break in type checking somewhere down the line. It's also true that things "just work" because the people that have been contributing so far understand the codebase well, and you have good unit tests to cover your work. In some cases people might not have as good an understanding of the overall codebase (yours truly being a good example), and its nice to have accurate static typing to hold your hand.

I'll also point out that the extra emits have an effect proportional to the number of calls to every in the program, not the number of items that pass through the every operator. A large sized program will have, let's say a few thousand invocations (generously) of the operator functions, so an extra local variable per shouldn't have any performance impact.

All this aside, I'll be removing the _this aliases for this PR and will open a separate issue to discuss the pros/cons of it.

benlesh · 2016-01-22T20:53:53Z

src/operator/scan.ts

@@ -13,14 +13,15 @@ import {errorObject} from '../util/errorObject';
 * @returns {Obervable} An observable of the accumulated values.
 */
 export function scan<T, R>(accumulator: (acc: R, x: T) => R, seed?: T | R): Observable<R> {
-  return this.lift(new ScanOperator(accumulator, seed));
+  let _this: Observable<T> = this;


Again, for this sort of thing, I fail to see the benefit to our end users. It's just going to add to the transpiled output, I believe.

masaeedu · 2016-01-27T05:00:51Z

@kwonoj, @Blesh Ok, I've nuked the _this changes. The discussion regarding what to do about Subject has stalled a bit, so I think the prudent thing to do is to skip fixing the lift signature on that and the derived websocket subject for now.

benlesh · 2016-02-02T17:38:33Z

@masaeedu can you rebase this PR please?

Also: just a note on PRs like this in general:

If it's internal code typings your changing (read: Nothing that a developer-user would interface with)...

My feeling is that <any> is fine, unless it's going to speed up the build to do more specific casting. (which I'm not sure about)... or unless it's going to add a lot to our development process to have things properly typed. (cc @david-driscoll @saneyuki @kwonoj on this note)

david-driscoll · 2016-02-05T20:12:34Z

Would love to see this one in fairly soon, working on additional operator signatures at the moment and ran into an issue were the changes ran into these same issues. @masaeedu 👍

masaeedu · 2016-02-05T20:22:09Z

Sorry about the delay folks. I'll be cleaning this up and rebasing on Saturday.

masaeedu · 2016-02-07T14:09:26Z

@Blesh Ok, rebased and removed most of the changes since they were controversial. The changes now are limited to removing redundant generics from Operator.call and Observable.lift and changing the minimum number of operators necessary to compile the project again.

This still leaves a number of operators where the internal typings (and in some cases user-facing typings) are wrong, but they still compile since this is any. Possibly these can be done in a different PR/skipped since low priority.

cc @kwonoj

luisgabriel · 2016-02-07T14:19:53Z

@masaeedu maybe it would be worth to create an issue listing the operators with wrong typing (especially the external ones) so we could try to fix it incrementally.

kwonoj · 2016-02-09T09:13:06Z

Sorry for delays, I'll start look into updated changes.

kwonoj · 2016-02-09T23:17:20Z

Change looks good to me, nice PR @masaeedu 😄

kwonoj · 2016-02-10T01:09:29Z

I'll check in this PR after leaving bit more to see if additional suggestions around, expecting later today to tomorrow morning. cc @david-driscoll @luisgabriel @Blesh for visibility.

luisgabriel · 2016-02-10T02:22:30Z

LGTM.

Thanks @masaeedu for this nice PR. It raised important discussions regarding the typings.

david-driscoll · 2016-02-10T02:44:27Z

LGTM 👍

kwonoj · 2016-02-10T07:07:31Z

Merged with f27902d, thanks for great PR @masaeedu !

lock · 2018-06-07T10:02:26Z

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.

luisgabriel reviewed Jan 18, 2016
View reviewed changes

luisgabriel reviewed Jan 20, 2016
View reviewed changes

kwonoj reviewed Jan 20, 2016
View reviewed changes

masaeedu mentioned this pull request Jan 22, 2016

Publishing to a lifted Subject is type-unsafe #1234

Closed

benlesh reviewed Jan 22, 2016
View reviewed changes

benlesh added the blocked label Feb 2, 2016

fix(typings): remove redundant generics from call<T, R> and lift<T, R>

89ec3c4

masaeedu force-pushed the typingFixes branch from 438e825 to 89ec3c4 Compare February 7, 2016 14:03

fix(typings): remove R from Operator.call, update operators accordingly

bade6a0

kwonoj removed the blocked label Feb 9, 2016

kwonoj added the PR: lgtm label Feb 9, 2016

kwonoj closed this Feb 10, 2016

masaeedu deleted the typingFixes branch February 12, 2016 22:02

lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018

Typing fixes #1214

Typing fixes #1214

Conversation

masaeedu commented Jan 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisgabriel commented Jan 18, 2016

masaeedu commented Jan 19, 2016

luisgabriel commented Jan 19, 2016

masaeedu commented Jan 19, 2016

kwonoj commented Jan 19, 2016

masaeedu commented Jan 19, 2016

kwonoj commented Jan 19, 2016

masaeedu commented Jan 19, 2016

masaeedu commented Jan 20, 2016

kwonoj commented Jan 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-driscoll commented Jan 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masaeedu commented Jan 20, 2016

kwonoj commented Jan 20, 2016

masaeedu commented Jan 20, 2016

masaeedu commented Jan 22, 2016

luisgabriel commented Jan 22, 2016

kwonoj commented Jan 22, 2016

masaeedu commented Jan 22, 2016

kwonoj commented Jan 22, 2016

masaeedu commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masaeedu commented Jan 27, 2016

benlesh commented Feb 2, 2016

david-driscoll commented Feb 5, 2016

masaeedu commented Feb 5, 2016

masaeedu commented Feb 7, 2016

luisgabriel commented Feb 7, 2016

kwonoj commented Feb 9, 2016

kwonoj commented Feb 9, 2016

kwonoj commented Feb 10, 2016

luisgabriel commented Feb 10, 2016

david-driscoll commented Feb 10, 2016

kwonoj commented Feb 10, 2016

lock bot commented Jun 7, 2018