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

feat(delayWhen): add delayWhen operator #1245

Closed
wants to merge 1 commit into from

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Jan 25, 2016

closes #1202

This PR implements delayWhen operator supports selector with subscription delay current delay operator doesn't supports from RxJS4.

@benlesh
Copy link
Member

benlesh commented Jan 25, 2016

Looks okay overall. I think we should try to get this method to use common logic such as subscribeToResult to enable things like Promise and Observablesque support.

@kwonoj
Copy link
Member Author

kwonoj commented Jan 25, 2016

Thanks. I'll give a try to refactor now to see if I can make it.

@benlesh
Copy link
Member

benlesh commented Jan 25, 2016

In fact, I bet there are quite a few more operators that should probably use that code path to ensure they support Promises and the like.

@kwonoj
Copy link
Member Author

kwonoj commented Jan 25, 2016

OK, refactored by using subscribeToResult with OuterSubscriber. I came to remember reason why I dropped it first time, it was due to manage subscription for some cases (like notifier does not emits but completes) was bit tricky. For this change I've ended up using Map with outer value index as key, any suggestion would be appreciated if you think there's better way.

@kwonoj kwonoj force-pushed the feat-delaywhen branch 2 times, most recently from eddd8bf to d2071f6 Compare January 25, 2016 22:32

class DelayWhenSubscriber<T, R> extends OuterSubscriber<T, R> {
private completed: boolean = false;
private delayNotifiers: Array<DelayNotiferSubContext<T>> = [];
Copy link
Member Author

Choose a reason for hiding this comment

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

now tried to use simple array instead of map, subscription deletion via lookup & splice.

@kwonoj kwonoj added the blocked label Jan 26, 2016
@kwonoj
Copy link
Member Author

kwonoj commented Jan 26, 2016

PR should be updated after #1252 checked in, marking as blocked for now.

@kwonoj
Copy link
Member Author

kwonoj commented Jan 27, 2016

updated PR to utilize innersub passed by notifyNext, also removes trycatcher as recent changes.

@kwonoj
Copy link
Member Author

kwonoj commented Feb 1, 2016

I'll check this in later today to see any other suggestions around.

@kwonoj
Copy link
Member Author

kwonoj commented Feb 1, 2016

actually noticed code was using old trycatcher, updated PR to remove it.

@kwonoj
Copy link
Member Author

kwonoj commented Feb 2, 2016

Merged with 17122f9.

@kwonoj kwonoj closed this Feb 2, 2016
@kwonoj kwonoj deleted the feat-delaywhen branch February 2, 2016 08:40
@kwonoj kwonoj restored the feat-delaywhen branch February 5, 2016 10:01
@kwonoj kwonoj deleted the feat-delaywhen branch February 5, 2016 10:01
@lock
Copy link

lock bot commented Jun 7, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
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.

Missing polymorphic behavior (delayWhen) in RxJS 5
2 participants