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

finally is not passing observable through properly #1672

Closed
johnpapa opened this issue Apr 29, 2016 · 19 comments · Fixed by #1673
Closed

finally is not passing observable through properly #1672

johnpapa opened this issue Apr 29, 2016 · 19 comments · Fixed by #1673
Assignees

Comments

@johnpapa
Copy link

johnpapa commented Apr 29, 2016

RxJS version:
rxjs 5 beta 6

Code to reproduce:

  getSessions() {
    this.spinnerService.show();
    return <Observable<Session[]>>this.http
      .get(sessionsUrl)
      .map(res => this.extractData<Session[]>(res))
      .catch(this.exceptionService.catchBadResponse)
      .finally(() => this.spinnerService.hide());
  }

See code example here ...
https://github.com/johnpapa/event-view/commit/da21a922faf74aa3f2b6fbf1af5d1d666419b24e#diff-a115014ed5c151b197295d50f1ce2a54R24

Expected behavior:
TypeScript's tsc should think getSessions should return Observable<Session[]>

Actual behavior:
TypeScript's tsc thinks getSessions is returning Observable<{}>

Additional information:

  • When I comment out the finally TypeScript is happy
  • When I cast the return type as <Observable<Session[]>> tsc is happy

Error message:

> tsc -p ./src/client

src/client/app/+sessions/session-list/session-list.component.ts(34,9): 
  error TS2322: Type '{}' is not assignable to type 'Session[]'.

cc // @david-driscoll @Blesh @robwormald @wardbell

@kwonoj kwonoj self-assigned this Apr 29, 2016
@kwonoj
Copy link
Member

kwonoj commented Apr 29, 2016

Think I found cause.

kwonoj added a commit to kwonoj/rxjs that referenced this issue Apr 29, 2016
@kwonoj
Copy link
Member

kwonoj commented Apr 29, 2016

@johnpapa , I believe this is fixed by latest changes in master branch. If you're ok please check with master, or it'll be published in next release. Thanks for catching this.

@benlesh
Copy link
Member

benlesh commented Apr 29, 2016

@johnpapa next release will be Monday unless I get a deluge of requests prior to ng-conf.

@johnpapa
Copy link
Author

trying it now.

just noticed i have this line ... which is not what you changed. i am on beta 6 (as it matches angular 2 beta 17)

   <T>(finallySelector: () => void): Observable<T>;

@kwonoj
Copy link
Member

kwonoj commented Apr 29, 2016

Yes, that's prior to master including changes. Next package will be published in next week as @Blesh mentioned, or for immediate trying you may need to pull down master branch.

@johnpapa
Copy link
Author

johnpapa commented Apr 29, 2016

image

src/client/app/+sessions/session-list/session-list.component.ts(34,9): 
  error TS2322: Type '{}' is not assignable to type 'Session[]'.

@johnpapa
Copy link
Author

johnpapa commented Apr 29, 2016

Same error :(

Reloaded the project in VS Code just in case. Re-ran tsc from terminal too.

@david-driscoll
Copy link
Member

worst case you could manually set the type .finally<Session[]>(() => this.spinnerService.hide());

@johnpapa
Copy link
Author

Right. I'm casting now. But I think this issue should be reopened as it is not fixed from.what I tried. See above

@kwonoj
Copy link
Member

kwonoj commented Apr 30, 2016

I just confirmed locally with master and it's working as expected. @johnpapa , would you be able to isolate by create small repo using bare Rx master only, without involving ng2?

@david-driscoll
Copy link
Member

I wonder if there is a nested rxjs folder that's getting picked up over a custom build.

I'm hanging out on the ReactiveX, OmniSharp and ASP.NET Core slacks if you want to chat a little more real time.

@johnpapa
Copy link
Author

i can try to do a smaller repo later, but for now here is the repo where i cast it. https://github.com/johnpapa/event-view/blob/master/src/client/app/shared/speaker-services/speaker.service.ts#L39-L46

@david-driscoll
Copy link
Member

Looks like catch might have an indirect problem as well, I'll see what I can do.

@david-driscoll
Copy link
Member

With the changes in that additional PR, I was able to do this (without the cast in front as well!)
image

I also made one other change to catchBadRequest

  catchBadResponse: <T>(errorResponse: any) => Observable<T> = (errorResponse: any) => {
    let res = <Response>errorResponse;
    let err = res.json();
    let emsg = err ?
      (err.error ? err.error : JSON.stringify(err)) :
      (res.statusText || 'unknown error');
    this.toastService.activate(`Error - Bad Response - ${emsg}`);
    // return Observable.throw(emsg); // TODO: We should NOT swallow error here.
    return Observable.of();
  };

@kwonoj
Copy link
Member

kwonoj commented Apr 30, 2016

Just checked in changes for catch as well.

@johnpapa
Copy link
Author

thanks for the help. it is not working in the repo i pointed at (event-view). I made those two changes in catch and finally files. But I am on beta 6, so maybe there are other things behind.

i can look more later. need to focus on some other things first. thanks tho

@bzuillsmith
Copy link

I am getting this problem as well even with the latest. I was running Beta 6 and now running Beta 9. I'm using Visual Studio Code.
An example of the call I'm making:

this.userService.doSomething() //returns Observable<any>
        .finally(
            () => this.status = ''
        ) // VS Code shows this returns Observable<{}>
        .subscribe(
            result => {
                this.x = result.x; //complains here because result is of type {}
            },
            error => this.errorMessage = error
        )

@kwonoj
Copy link
Member

kwonoj commented Jun 19, 2016

@bzuillsmith I'll try with master, meanwhile would you mind to share complete snippet to able to reproduce issue? it might be possible with specific cases only, would like to confirm it as well.

@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 a pull request may close this issue.

5 participants