Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(content): use observables instead of unsubscribe #10098

Closed
wants to merge 1 commit into from
Closed

refactor(content): use observables instead of unsubscribe #10098

wants to merge 1 commit into from

Conversation

masimplo
Copy link
Contributor

This pull request is in reference to 3a718de#commitcomment-20529331

Make use of the RxJS API to clear subscriptions
when they are no longer needed, instead of using
subscription references to unsubscribe. take(1)
will clear the subscription after it has been used
exactly 1 times. TakeUntil will clear the subscription
when an event is emitted on the passed observable,
ie. ngOnDestroy has been called.

Make use of the RxJS API to clear subscriptions
when they are no longer needed, instead of using
subscription references to unsubscribe. take(1)
will clear the subscription after it has been used
exactly 1 times. TakeUntil will clear the subscription
when an event is emitted on the passed observable,
ie. ngOnDestroy has been called.
masimplo referenced this pull request Jan 19, 2017
#10050)

* fix(content): unsubscribe from observables on destroy

* fix(content): scroll is initialized before subscribing

fixes #9593
fixes #10045

* fix(content): unset viewCtrl subscribers on destroy
@manucorporat
Copy link
Contributor

@masimplo the team is refactoring a lot stuff for the 3.0 release related with tree-shaking and lazy loading, we will review this PR after 3.0 branch is merged into master.

@masimplo
Copy link
Contributor Author

Thanks for letting me know. Looking forward to 3.0!

@manucorporat
Copy link
Contributor

@masimplo I am not working in the "build" team, and i am kind of worried about adding dependencies... it might break some tree-shaking stuff my colleagues are working on. I prefer to wait :)

But thanks for PR, i would love to use this a lot more across the framework, not only in content.ts

@Ionitron
Copy link
Collaborator

Hello and thank you for contributing to Ionic! We have been working on porting all of the Ionic components to web components and have recently updated master to reflect this. This significant change has caused this pull request to break. While we really appreciate the time and effort you put into creating this, we are not able to merge it because of the newly introduced conflicts. We are extremely sorry about this. We will not be merging any more features in to v3. If this is a feature and you have the time, please resubmit this PR against the master branch. If this is a critical security issue in v3, we would greatly appreciate it if you would resubmit the PR against the new v3 branch. Thanks so much for your time!

@Ionitron Ionitron closed this Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants