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(if): add static Observable.if creation operator. #1348

Merged
merged 1 commit into from
Mar 10, 2016

Conversation

trxcllnt
Copy link
Member

Addresses #1343

@@ -39,8 +39,10 @@ import './add/observable/fromArray';
import './add/observable/fromEvent';
import './add/observable/fromEventPattern';
import './add/observable/fromPromise';
import './add/observable/if';
Copy link
Member

Choose a reason for hiding this comment

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

is this for kitchensink only? not in rx?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kwonoj in Rx4 Observable.if is in the "experimental" package, so I only put it into KitchenSink. I still question why we distinguish between "core" and "kitchen sink" operators in the first place.

return elseSource.subscribe(subscriber);
} else {
subscriber.complete();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about moving this if to a separate function? I know this is not a very performance sensitive operator, but we can have improvements anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, that's alright with me.

Copy link
Member

Choose a reason for hiding this comment

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

@trxcllnt , would you be update PR as suggested, or proceed to check in as-is?

@benlesh
Copy link
Member

benlesh commented Mar 1, 2016

Blocking this while #1364 is merged.

@benlesh
Copy link
Member

benlesh commented Mar 2, 2016

@trxcllnt I think this needs rebased/refactored to move the tests to TypeScript.

@trxcllnt trxcllnt force-pushed the static-if-operator branch 2 times, most recently from 5efb402 to 297722c Compare March 2, 2016 19:53
@trxcllnt
Copy link
Member Author

trxcllnt commented Mar 2, 2016

@Blesh done

@benlesh
Copy link
Member

benlesh commented Mar 9, 2016

@trxcllnt this still appears to be in need of a rebase. I'm sure that something just moved around you, but can you hit it again? After that, myself or @kwonoj will merge it in ASAP.

@trxcllnt
Copy link
Member Author

@Blesh rebased again good 2 go

@kwonoj
Copy link
Member

kwonoj commented Mar 10, 2016

I'll check this in today. :)

@kwonoj kwonoj merged commit f7ff7ec into ReactiveX:master Mar 10, 2016
@kwonoj
Copy link
Member

kwonoj commented Mar 10, 2016

Merged with f7ff7ec, thanks @trxcllnt including several rebase :)

@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.

None yet

4 participants