Support pipable cancelOnDestroy and tag operators #117
Conversation
|
|
||
| export function cancelOnDestroy<T>(instance: OnDestroy, manualDestroy?: Observable<any>): (source: Observable<T>) => Observable<T> { | ||
| return (source: Observable<T>) => { | ||
| if (instance.ngOnDestroy) { |
There was a problem hiding this comment.
not really a review comment, more of an advise, feel free to ignore:
I usually prefer to use a more defensive programming approach using guard clauses:
https://refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html
can remove one level of indentation by replacing the if logic like so:
if (!instance.ngOnDestroy) {
return source;
}
.... rest of code here, no need else after a return.
There was a problem hiding this comment.
okay, I don't mind
| @@ -0,0 +1,50 @@ | |||
| import { Observable } from 'rxjs/Observable'; | |||
There was a problem hiding this comment.
creation method should be imported like so:
import { Observable, Subject, asapScheduler, pipe, of, from, interval, merge, fromEvent } from 'rxjs';
see:
https://github.com/ReactiveX/rxjs/blob/master/docs_app/content/guide/v6/migration.md#import-paths
There was a problem hiding this comment.
correcting my previous comment.
There was a problem hiding this comment.
merge is not available for import from 'rxjs' in v5.5.6 yet
There was a problem hiding this comment.
I see, I thought this work was done as part of v6 migration. my bad.
|
|
||
| if (!(<EnhancedOnDestroy>instance).__ngOnDestroySource__) { | ||
| (<EnhancedOnDestroy>instance).__ngOnDestroySource__ = new Subject(); | ||
| (<EnhancedOnDestroy>instance).__ngOnDestroy__ = instance.ngOnDestroy; |
There was a problem hiding this comment.
I think this might be a good place to use Symbol, as I don't think we want this property to be enumerable. plus it will make sure there are no collisions.
There was a problem hiding this comment.
hmm, never used it before :)
const ngOnDestroySource = Symbol('ngOnDestroySource');
const ngOnDestroy = Symbol('ngOnDestroy');
interface EnhancedOnDestroy extends OnDestroy {
[ngOnDestroySource]: Subject<string>;
[ngOnDestroy]: () => void;
}
//...
if (!instance[ngOnDestroySource]) {
instance[ngOnDestroySource] = new Subject();
instance[ngOnDestroy] = instance.ngOnDestroy;
//...
something like this, right? (I haven't tried it in the runtime yet, just guessing in the comments)
There was a problem hiding this comment.
exactly. The symbol data structure is built for use cases like these. I would consult @esakal first, as I think it requires a polyfill for IE.
There was a problem hiding this comment.
Hi, It is nice talking about advanced techniques :)
I think this might be a good place to use Symbol, as I don't think we want this property to be enumerable. plus it will make sure there are no collisions.
It will prevent collisions as you mentioned and will be ignored so it fit the purpose.
I think it requires a polyfill for IE
We already have this polifill enabled here
There was a problem hiding this comment.
Okay, apparently it doesn't work yet in typescript v2.5.3 we have in kaltura-ng.
It relates to this issue microsoft/TypeScript#5579 and it was closed by this PR microsoft/TypeScript#15473 which was included into v2.7
@esakal We can leave it as a plain property for now or wait for upgrade of the TS (if it was planned), what do you prefer?
|
Already implemented in the https://github.com/kaltura/kaltura-ng/tree/ng6 branch (6dbfe4f) I've missed one important thing, those operators shouldn't brake subscription chain and my implementation return a new observable instead of patching original one. @omridevk fyi |
PR information
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
What is the new behavior?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information
Where should the reviewer start?
How should this be manually tested?
Any background context you want to provide?
This change is