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(Observable): initial IObservable interface #1741

Closed
wants to merge 1 commit into from
Closed

feat(Observable): initial IObservable interface #1741

wants to merge 1 commit into from

Conversation

vankeer
Copy link

@vankeer vankeer commented Jun 2, 2016

This PR is not meant to be merged, but to gauge for your opinion on this.
If approved, I can make the changes for the other operators and methods as well.

Description:

An interface for Observables.

This fixes a problem we were having when npm linking a library exposing methods that return Observables.

For example, when you npm link ng2-example-library (ng2-example-library) in ng2-example-consumer, you get this:

src\app\repository\repository.service.ts(14,10): error TS2322: Type 'Observable<any[]>' is not assignable to type 'Observable<any[]>'.
Property 'source' is protected but type 'Observable' is not a class derived from 'Observable'.

Impact:

Quite a lot of operator signatures would have to be adapted to accommodate this change, as well as changes by the user would be required (Observable > IObservable). We could avoid the latter by renaming the interface to Observable and the class to something else.

@kwonoj
Copy link
Member

kwonoj commented Jun 2, 2016

I think this is related with issue #1248, currently bit on hold to clarify / ensure some of usecases. /cc @david-driscoll as well.

@david-driscoll
Copy link
Member

What version of the TypeScript compiler are you on? I believe a lot of these issues were fixed already.

microsoft/TypeScript#6731 and microsoft/TypeScript#6786

If something still remains, it's likely something that can be fixed without a huge sweeping change here (hopefully anyhow).

@vankeer
Copy link
Author

vankeer commented Jun 3, 2016

I am using version 1.8.10 so that should be fine. I am still looking for solutions other than changing the return type to an interface.

@kwonoj
Copy link
Member

kwonoj commented Jun 4, 2016

I'd like to suggest create issue first to illustrate issues with latest master to be discussed. While introducing interface to observable is on discussion, it has somewhat different goals this PR tries to achieve and possibly can take longer time. If current problem / issue can be fixed in other way, it could be introduced separately in current shape of codebases.

@vankeer
Copy link
Author

vankeer commented Jun 6, 2016

Thanks @kwonoj and @david-driscoll for your responses.
Given that this is indeed quite a huge refactoring, I have opened issue #1744 and will close this PR.

@vankeer vankeer closed this Jun 6, 2016
@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.

3 participants