Skip to content
This repository has been archived by the owner on Nov 16, 2021. It is now read-only.

Various tweaks #28

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Various tweaks #28

wants to merge 4 commits into from

Conversation

isoden
Copy link
Contributor

@isoden isoden commented Jan 5, 2018

I changed it to be simpler.
please review :)

Thanks!

private _isPlatformBrowser: boolean = isPlatformBrowser(this.platformId);

private _beforeInit: Subject<void> = new Subject<void>();
private _onDestroy: Subject<void> = new Subject<void>();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of _beforeInit and _onDestroy, exactly?

return merge(...sources)
.pipe(
takeUntil(this._beforeInit),
takeUntil(this._onDestroy),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty unfamiliar with some rxjs practices. Could you explain what's going on here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this link will answer both your questions:
http://brianflove.com/2016/12/11/anguar-2-unsubscribe-observables/
takeUntil() is a practice to make sure a subscribe gets unsubscribed for sure when some action occurs.
the action can be defined with Subject, and then when you call Subject.next() Subject.complete() (isoden added this in lines 63-64 of this pull request) you make the action happens, which in turn makes the unsubscribe happen for the subscribe that was registered to this Subject (with takeUntil).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't you just add a takeUntil(merge(this._beforeInit, this._onDestroy))
instead of double takeUntils ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants