Proposal : remove circular reference using ambient merge #909
Conversation
- remove circular references using microsoft/TypeScript#3332, parents are references children via ambient interfaces and let compiler merges it
| @@ -89,7 +90,7 @@ export class Observable<T> implements CoreOperators<T> { | |||
| let subscriber: Subscriber<T>; | |||
|
|
|||
| if (observerOrNext && typeof observerOrNext === 'object') { | |||
| if (observerOrNext instanceof Subscriber || observerOrNext instanceof Subject) { | |||
| if (observerOrNext instanceof Subscriber) { //|| observerOrNext instanceof Subject) { | |||
There was a problem hiding this comment.
This is part where regresses #863, ambient declaration of Subject cannot check its type via instanceof since it's interface.
There was a problem hiding this comment.
How about
if (observerOrNext instanceof Subscriber ||
(typeof (<Subject>observerOrNext).next === 'function' && typeof (<Subject>observerOrNext).subscribe === 'function')) {There was a problem hiding this comment.
Only reason for the explicit <Subject> is to make the code tool-friendly.
There was a problem hiding this comment.
Yes, need to cast <any> instead of Subject (since compile time imported ambient declaration does not have interfaces) but this works, thanks for pointing out. I was just spending time in opposite way of bringing interface check availability to runtime..
There was a problem hiding this comment.
I have an alternative PR for this particular bit here #912
|
Due to regression, this PR does not passes travis and it's expected. |
| @@ -0,0 +1 @@ | |||
| export interface ConnectableObservable<T> {} | |||
There was a problem hiding this comment.
maybe this declarations could be single module such as
ambient.ts
export interface A{}
export interface B{}
export interface C{}
to allow single import to declarations like
import {A,B,C} from 'ambient'
There was a problem hiding this comment.
I agree, make these all into one file.
|
Note : this change has side effect of reducing build time speed. On travis, numbers are below (may vary per run though) without PR(cjs) with PR(cjs) |
|
I'll leave this PR as is for now to get opinions of direction itself, once it's agreed to go will update PR accordingly. |
|
Yeah, I think we can probably close this one for now, since we no longer have the circular reference issue. I really appreciate the thought and hard work here though. It's an idea I'll have to remember if we do run into the circular reference issue again. |
|
Thanks again, @kwonoj |
|
It is cool to have changes landed to resolve issue. Thanks for taking time to reviewing this, @Blesh :) |
Proposal : not to be merged yet
This PR is proposal to resolve #899, via ambient declaration merging introduced in typescript (microsoft/TypeScript#3332).
In this PR, for any parents requires to have declaration of inherited childs, it imports ambient declarations instead of actual implementation of child to remove circular dependencies. Since reference to child from parents is mostly for type declaration only, this doesn't affect actual functionality in general.
Still for some cases functionality's breaking, such as this PR regresses #863 due to failed to check type of
Subjectsince ambient declaration doesn't allow type checking. Yet this is proposal, PR does not include those refactoring but only include necessary changes.