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

fix(never): Observable.never() Observable.empty() .ignoreElements () now return Observable<never> #2691

Merged
merged 1 commit into from Jun 26, 2017
Merged

fix(never): Observable.never() Observable.empty() .ignoreElements () now return Observable<never> #2691

merged 1 commit into from Jun 26, 2017

Conversation

jayphelps
Copy link
Member

fixes #2640

BREAKING CHANGE: Previously, Observable.never() Observable.empty()
and the .ignoreElements() operator all returned Observable<T> which
was incorrect since they actually never emit anything. Now they all
return Observable<never> (never was added in TS 2.0 as a special
type)

kwonoj
kwonoj reviewed Jun 22, 2017
View reviewed changes
src/observable/EmptyObservable.ts Outdated
}

static dispatch<T>(arg: DispatchArg<T>) {
static dispatch(arg: DispatchArg<never>) {
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't it better to make DispatchArg as non-generic in line 6? (also making this fn to private and not export dispatcharg would be preferred)

Copy link
Member Author

Choose a reason for hiding this comment

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

wasn't clear if DispatchArg was used elsewhere since it was exported, but certainly can 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

it will basically just mean I make subscriber: Subscriber<never> instead of subscriber: Subscriber<T>

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.734% when pulling afc7734 on jayphelps:never into 0506ea0 on ReactiveX:master.

@jayphelps jayphelps force-pushed the never branch 3 times, most recently from d6f40e2 to 49a339e Compare June 22, 2017 18:58
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.734% when pulling 49a339e on jayphelps:never into 0506ea0 on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.734% when pulling 49a339e on jayphelps:never into 0506ea0 on ReactiveX:master.

@david-driscoll
Copy link
Member

You should probably target the next branch instead of master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.734% when pulling 49a339e on jayphelps:never into 0506ea0 on ReactiveX:master.

@kwonoj kwonoj changed the base branch from master to next June 22, 2017 19:16
@kwonoj
Copy link
Member

kwonoj commented Jun 22, 2017

(I've changed base to next)

david-driscoll
david-driscoll approved these changes Jun 22, 2017
View reviewed changes
Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

lgtm

kwonoj
kwonoj requested changes Jun 22, 2017
View reviewed changes
Copy link
Member

@kwonoj kwonoj left a comment

Choose a reason for hiding this comment

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

Guess some change comes by changing base 🤔 might need correct rebase.

@kwonoj
Copy link
Member

kwonoj commented Jun 22, 2017

ah, change itself LGTM

@jayphelps
Copy link
Member Author

jayphelps commented Jun 22, 2017 •
edited

@kwonoj issue is that next doesn't have everything from master yet, so my branch is bringing them along for the ride. I can prolly reset my branch to the same as next then cherry pick my commit in, or next can be updated to the latest master before this is merged. LMK what ya'll would like

i'm going to reset my branch to next and cherry pick it in, you all can upgrade next to latest master if you want..or not. If you don't, it may or may not make doing so later harder

@jayphelps
fix(never): Observable.never() Observable.empty() and .ignoreElements…
d0ca8c2 
…() now return Observable<never>

fixes ReactiveX#2640

BREAKING CHANGE: Previously, `Observable.never()` `Observable.empty()`
and the `.ignoreElements()` operator all returned `Observable<T>` which
was incorrect since they actually never emit anything. Now they all
return `Observable<never>` (`never` was added in TS 2.0 as a special
type)
@jayphelps
Copy link
Member Author

@kwonoj reset to next then cherry picked commit into it.

kwonoj
kwonoj approved these changes Jun 22, 2017
View reviewed changes
Copy link
Member

@kwonoj kwonoj left a comment

Choose a reason for hiding this comment

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

🚢

@coveralls
Copy link

coveralls commented Jun 22, 2017 •
edited

Coverage Status

Coverage remained the same at 97.734% when pulling 49a339e on jayphelps:never into 0506ea0 on ReactiveX:master.

@jayphelps
Copy link
Member Author

Need anything else for merge?

@david-driscoll david-driscoll merged commit 4dfeb9f into ReactiveX:next Jun 26, 2017
@kwonoj kwonoj mentioned this pull request Jul 12, 2017
Closed
@101100 101100 mentioned this pull request Jul 30, 2017
Closed
This was referenced Oct 12, 2017
Closed
Closed
@peaBerberian peaBerberian mentioned this pull request Nov 23, 2017
Merged
@mernen mernen mentioned this pull request Jan 2, 2018
Closed
@kwonoj kwonoj mentioned this pull request Apr 24, 2018
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

@kwonoj kwonoj kwonoj approved these changes

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

None yet
4 participants
@jayphelps @coveralls @david-driscoll @kwonoj