fix: make TS type def of VirtualAction compatible with TS >= 2.6

[thetrompf]

Description:
I ran into a problem when using RxJS 5.5.11 with typescript newer than 2.6.
The type def for VirtualAction from rxjs/scheduler/VirtualTimeScheduler is not compatible with newer typescript, so I cannot compile my project.

ERROR in /home/---/workspace/project/node_modules/rxjs/scheduler/VirtualTimeScheduler.d.ts                               
ERROR in /home/---/workspace/project/node_modules/rxjs/scheduler/VirtualTimeScheduler.d.ts(24,15):                       
TS2416: Property 'work' in type 'VirtualAction<T>' is not assignable to the same property in base type 'AsyncAction<T>'.                           
  Type '(this: VirtualAction<T>, state?: T | undefined) => void' is not assignable to type '(this: AsyncAction<T>, state?: T | undefined) => void'.
    The 'this' types of each signature are incompatible.                                                                                           
      Type 'AsyncAction<T>' is not assignable to type 'VirtualAction<T>'.                                                                          
        Property 'index' is missing in type 'AsyncAction<T>'.                                                                                      

Related issue (if exists):
Fixes: #3031

[cartant]

I'm not sure why this change to stable is different to the changes that were made in this commit. In which, the action constructors were changed to accept the base Action - subsequently renamed to SchedulerAction.

If this is something that needs to be fixed in stable, shouldn't the problem be fixed in a similar manner?

[thetrompf]

I don't know what branch I should "compare" it to. It is a type fix for v5, and master is v6. The v6 have already "fixed" this in a different way, because the behavior has changed. But I will not be able to migrate my company's codebase to v6 any time soon. So hopefully you're still patching up v5.

[cartant]

So are you saying that the changes like this one cannot be used or don't solve the problem with TypeScript 2.0? I'm wondering why your PR fixes the problem a different way, that's all.

[thetrompf]

No they cannot, due to stricter type checking in TypeScript >= 2.6. When extending AsyncAction<T> and overwriting the work property/method, TS yells becuase the work is incompatible with the work property of AsyncAction.

class AsyncAction<T> {
    protected work: (this: AsyncAction<T>, state?: T) => void;
}

class VirtualAction<T> extends AsyncAction<T> {
    protected work: (this: VirtualAction<T>, state?: T) => void; // Error
}

The this binding is incompatible. But if you you change the definition in AsyncAction<T> to bind this to this TS type this it means, this the this context of the work method will be the same as the class it lives on, regardless if it is extended and overwritten in another class like VirtualAction<T>.

class AsyncAction<T> {
    protected work: (this: this, state?: T) => void;
}

class VirtualAction<T> extends AsyncAction<T> {
    protected work: (this: this, state?: T) => void; // works - compatible and the this binding refers to VirtualAction as it should.
}

[thetrompf]

Hmm.. now I understand what you mean. The PR you're refering to is not available in v5 right? It is only available in v6?

[cartant]

My understanding is that this commit fixed the problem in master - which uses typescript@latest - and did so by changing the type of the work function passed to the constructor:

constructor(protected scheduler: AsyncScheduler,
            protected work: (this: Action<T>, state?: T) => void) {
  super(scheduler, work);
}

constructor(protected scheduler: QueueScheduler,
            protected work: (this: Action<T>, state?: T) => void) {
  super(scheduler, work);
}

Is there a reason why similar changes cannot be applied to stable - in this PR - to fix the problem in a similar manner? Unless there is a problem with similar changes being made to stable, I think it would be undesirable to have the problem solved in a manner that differs from the solution used in master.

[thetrompf]

You're correct sir. I thought the fix only "applies" to v6. But sure, the fix to stable, if you are up for pathing up v5, should be made in the same manner.

Any suggestions how to move forward? :)

[cartant]

Whether or not it's something that really needs to be fixed in stable is yet to be decided. It's not up to me and there is work involved in creating a new release, etc.

Given that the problem can - it seems - be worked around using skipLibCheck (which, IMO, is an entirely reasonable compiler option to be using) it might not be sufficiently critical to warrant a release.

However, if the problem is going to be fixed in v5 (stable) it should be fixed in the same way that it was in v6.

[cartant]

Thanks for the effort you've put into the PR, BTW.

[cartant]

@benlesh What do you think of the problem this PR seeks to address? Is it something that users of v5 should just work around using skipLibCheck or should it be fixed?

[thetrompf]

Thanks a lot for your feedback. It is totally reasonable, and I do understand the complication in creating new releases, and futhermore patching up and maintaining "older" versions.

Do you know the full extend of the skipLibCheck implications?

Does it mean if I interface with let's say the VirtualAction type, that I won't get type errors all together (I can pass anything to library functions) and arguments will not be typechecked against rxjs types, or does it mean "just" mean that the type defs of the libraries won't break the compilation if they're not sound internally?

[cartant]

I use skipLibCheck everywhere.

My understanding is that it skips the checking of the internals of packages within node_modules, but any interface between the application and the packages within node_modules is checked.

As stated in the docs that accompanied its release of skipLibCheck in TypeScript 2.0, the declaration files within node_modules should already have been checked (when the packages in question were published):

When a program includes large declaration files, the compiler spends a lot of time type checking declarations that are already known to not contain errors, and compile times may be significantly shortened by skipping declaration file type checks.

[thetrompf]

Thanks a lot, it even sounds like something that you always want. :)

[benlesh]

We'll discuss this at the core team meeting.

My gut says to leave it as is, and try to get people to move to v6 (for a variety of reasons).

[benschulz]

The position of the TypeScript team is that "skipLibCheck should not be used to mask errors".

[benlesh]

I'm on the fence here. v5 was only ever targeting TS 2.0 or 2.1... At the same time, this appears to be a non-breaking change.

[benlesh]

I suppose, since it's not a breaking change for TS 2.0 and 2.1 users, and it solves a problem for people using newer versions of TypeScript, we can merge it... but I'd recommend getting off of 5.5 very soon.