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

fix(subject): Fixes signature of Subject::lift #2540

Merged
merged 1 commit into from
Jun 14, 2017
Merged

fix(subject): Fixes signature of Subject::lift #2540

merged 1 commit into from
Jun 14, 2017

Conversation

hearnden
Copy link

Related issue (if exists):
#2539

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.687% when pulling c6228f2 on hearnden:fix-subject-lift into ee1ce30 on ReactiveX:master.

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.

@hearnden thanks for the Pull Request, the change looks good to me.

We will have to discuss if it's a breaking change... honestly it could very well be a breaking change for some environments, so we'll have to decide when to bring this in.

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.

Looks good to me. Let's discuss today's meeting for versions.

@benlesh
Copy link
Member

benlesh commented Apr 12, 2017

@david-driscoll @kwonoj This might be breaking, so let's punt it to the next major. It's not a bug that should effect common uses cases, so we can wait a little bit.

@staltz
Copy link
Member

staltz commented Apr 13, 2017

LGTM

@benlesh benlesh changed the base branch from master to next June 14, 2017 20:54
@Igorbek
Copy link
Contributor

Igorbek commented Jun 14, 2017

With typescript@next this signature mismatch is being caught by compiler that prevents using rxjs completely. So I would not say this is a breaking change.

@benlesh
Copy link
Member

benlesh commented Jun 14, 2017

@Igorbek right now 6.0.0-alpha.0 is targeting TS 2.3... I'll be updating that to TS 2.4 RC in short order. That should level this out for you. However, the stable branch will stay at 2.0, as there were breaking changes required to update to 2.1. It's better to not break anyone than make a few people happy.

@benlesh benlesh merged commit 3003614 into ReactiveX:next Jun 14, 2017
GoshaFighten added a commit to GoshaFighten/dx-ngx-application that referenced this pull request Jun 27, 2017
@karptonite
Copy link

FYI, TypeScript has released 2.4; this issue is now caught by the compiler, so it won't be possible for anyone using rxjs version 5 to upgrade to the latest TypeScript.

@david-driscoll
Copy link
Member

Can you check if turning on skipLibCheck in your tsconfig.json will stop this error from happening? It should be a stop gap change until we can get 6.0 out.

@karptonite
Copy link

karptonite commented Jun 27, 2017

@david-driscoll Unfortunately, that doesn't help. While that gets rid of the error the compiler finds in Subject directly, it does nothing for all of the errors in my code that arise from using Subject.

@tetsuharuohzeki
Copy link
Contributor

@karptonite asObservable() method might help you if your code try to assign Subject<T> to the variable/return value which expect Observable<T>.

@karptonite
Copy link

@saneyuki Thanks! I think in this case, I'm better off just staying on ts 2.3.4 until the official fix comes.

@benjamincombes
Copy link

It would be really great if it could be released as soon as possible, as it affects Angular users too that can't upgrade to TypeScipt 2.4: angular/angular#17800

@villanuevadani
Copy link

I managed to make it work with RxJS fixed at 5.4.1 and TS at 2.3.4

@OliverJAsh
Copy link
Contributor

For all TypeScript users who want to upgrade to 2.4.x but don't want to use an alpha version of TypeScript, and don't want to upgrade to the next major version, is it possible to apply this change to v5 and release a new patch version? Of course this may not be possible if it's considered a breaking change.

@Igorbek
Copy link
Contributor

Igorbek commented Jun 29, 2017

My hack was to automatically patch rxjs typings on postinstall script to change 'lift<R>(operator: Operator<T, R>): Observable<T> to lift<R>(operator: Operator<T, R>): Observable<R>.
ps. I still cannot understand why the bugfix considered a breaking change.

@Meligy
Copy link

Meligy commented Jun 29, 2017

I also think this fix should not be considered a breaking change, and would make total sense as a bugfix release version not even minor.

@JemiloII
Copy link

JemiloII commented Jul 3, 2017

This should just be a bugfix patch fix for 5.x.x. All we are doing is fixing the typings for the Typescript compiler to understand how to read and compile the code.

@fletchsod-developer
Copy link

fyi the fix already landed on RxJS 5.4.2 (changelog), you can either close this issue or leave it open.

@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.