fix(Observable): align type definition of subscriber with Observable #2196
Conversation
|
What would be concise example to test out if this causes not regression? sometimes there are regression even build passes correctly (like recent changes to |
kwonoj
force-pushed
the
fix-observable-type
branch
from
0e2afc5 to
e52d014
Compare
|
I'm marking it as blocked for review approval / and sound test plan to verify non-regression, non-breaking. |
|
The change looks good... we'll just need to ensure it's non-breaking so we can figure out where it lands. |
|
Agreed. Need to figure out some way to test.. /cc @david-driscoll for borrowing some idea. |
|
Enabling #2202 into all current test cases will unblock this PR for most common usecase. |
| @@ -37,7 +37,7 @@ export class Observable<T> implements Subscribable<T> { | |||
| * can be `next`ed, or an `error` method can be called to raise an error, or | |||
| * `complete` can be called to notify of a successful completion. | |||
| */ | |||
| constructor(subscribe?: <R>(this: Observable<T>, subscriber: Subscriber<R>) => TeardownLogic) { | |||
| constructor(subscribe?: (this: Observable<T>, subscriber: Subscriber<T>) => TeardownLogic) { | |||
There was a problem hiding this comment.
I've suspected this could be troublesome at some point, but I'm sure I forgot about just after I saw it.
|
Okay... so this looks good, but it reminds me that we're actually leaking an implementation detail via TypeScript. We don't want to encourage our external users to use anything other than the Observer interface. Subscriber is more of an implementation thing for our library. |
|
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. |
Description:
This PR align type of subscriber param to
<T>, same as observable.Related issue (if exists):