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.

Sign up for GitHub

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

Jump to bottom

feat(operators): Use lift in the operators that don't currently use lift. #1941

Merged
merged 1 commit into from
Sep 15, 2016
Merged

feat(operators): Use lift in the operators that don't currently use lift. #1941

merged 1 commit into from
Sep 15, 2016

Conversation

trxcllnt
Copy link
Member

Description:
Use lift in the operators that don't currently use lift.

Related issue (if exists):
#1829

jayphelps
jayphelps reviewed Sep 13, 2016
View reviewed changes
src/operator/merge.ts
@@ -149,7 +148,7 @@ export function mergeStatic<T, R>(...observables: Array<ObservableInput<any> | S
concurrent = <number>observables.pop();
}

if (observables.length === 1) {
if (scheduler === null && observables.length === 1) {
Copy link
Member

Choose a reason for hiding this comment

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

did this fix an unreported bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh weird, that must have slipped in when I branched from master. yes, it's a bug fix that I was tracking down earlier today but didn't get to finish writing a test for. I can try to remove it from this change set, or alternatively add the test I was going to write and push again :[

Copy link
Member Author

Choose a reason for hiding this comment

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

added a test for this

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0004%) to 97.129% when pulling 241787e on trxcllnt:lift-everywhere into 070e480 on ReactiveX:master.

jayphelps
jayphelps reviewed Sep 13, 2016
View reviewed changes
src/Rx.ts
@@ -150,7 +150,6 @@ export {Subscriber} from './Subscriber';
export {AsyncSubject} from './AsyncSubject';
export {ReplaySubject} from './ReplaySubject';
export {BehaviorSubject} from './BehaviorSubject';
export {MulticastObservable} from './observable/MulticastObservable';
Copy link
Member

@jayphelps jayphelps Sep 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This will require a BREAKING CHANGE note, if accepted. It's publicly available in all builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

bummertown. I'll amend the commit message.

@jayphelps
Copy link
Member

jayphelps commented Sep 13, 2016 •
edited
Loading

Edit: I think this is wrong, read my comment after this one

Maybe I'm missing something but doesn't this break static operator usage, which leads me to believe we're not currently testing them like that 😱😱😱.

e.g. static merge just references the instance operator function.

Did you have thoughts on the somewhat related issue of static methods not using lift?
#1829 (comment) and more directly #1876

@jayphelps
Copy link
Member

jayphelps commented Sep 13, 2016 •
edited
Loading

I totally missed that Observable.merge aka static merge (and all the others I've checked so far) does not use the prototype operator function, instead using the static one e.g. mergeStatic which doesn't rely on this. So perhaps I'm drunk and this isn't an issue. Though I still would like static methods to lift too, but we could in fact punk on that regarding this PR.

@trxcllnt
Copy link
Member Author

@jayphelps I'm also keen to update the static methods to use a creation/conversion method, but I wanted to get the instance methods out of the way first, as I imagine it'll be a bit before the static method is finalized.

jayphelps
jayphelps reviewed Sep 13, 2016
View reviewed changes
src/operator/concat.ts
if (scheduler === null && observables.length === 1) {
return <Observable<R>>observables[0];
}

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 another one that slipped in or?

Copy link
Member Author

Choose a reason for hiding this comment

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

heh yep

Copy link
Member Author

Choose a reason for hiding this comment

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

added a test for this

@trxcllnt trxcllnt force-pushed the lift-everywhere branch 2 times, most recently from 49f5a70 to 5866d53 Compare September 13, 2016 06:32
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.144% when pulling 5866d53 on trxcllnt:lift-everywhere into 070e480 on ReactiveX:master.

@jayphelps
Copy link
Member

jayphelps commented Sep 15, 2016 •
edited
Loading

needs rebase.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.144% when pulling 5b68773 on trxcllnt:lift-everywhere into cd953b1 on ReactiveX:master.

@jayphelps
Copy link
Member

@trxcllnt I forgot to remind you: this needs a BREAKING CHANGE added to the commit for the removal of MulticastObservable.

@trxcllnt
feat(operators): Use lift in the operators that don't currently use l…
68af9ef 
…ift.

BREAKING CHANGE:  Removes MulticastObservable subclass in favor of a MulticastOperator.
@trxcllnt
Copy link
Member Author

@jayphelps done

@coveralls
Copy link

coveralls commented Sep 15, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 97.144% when pulling 68af9ef on trxcllnt:lift-everywhere into cd953b1 on ReactiveX:master.

@jayphelps
Copy link
Member

jayphelps commented Sep 15, 2016 •
edited
Loading

LGTM, thanks dudebro! 👏

@jayphelps jayphelps merged commit 6bd3423 into ReactiveX:master Sep 15, 2016
@trxcllnt trxcllnt mentioned this pull request Nov 15, 2017
Closed
@lock
Copy link

lock bot commented Jun 6, 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 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jayphelps jayphelps jayphelps approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@trxcllnt @coveralls @jayphelps