Skip to content

feat(http): xhr error listener invokes onError - #2667

Closed
PatrickJS wants to merge 1 commit into
angular:masterfrom
PatrickJS:patch-8
Closed

PatrickJS wants to merge 1 commit into
angular:masterfrom
PatrickJS:patch-8

Conversation

@PatrickJS

Copy link
Copy Markdown
Contributor
    this._xhr.addEventListener('error', (err) => {
      var responseOptions = new ResponseOptions({body: err, type: ResponseTypes.Error});
      if (isPresent(baseResponseOptions)) {
        responseOptions = baseResponseOptions.merge(responseOptions);
      }
      ObservableWrapper.callThrow(this.response, new Response(responseOptions))
    });

Review on Reviewable

@PatrickJS
PatrickJS force-pushed the patch-8 branch 2 times, most recently from 8f1d040 to 7db005d Compare June 21, 2015 23:42
@tbosch

tbosch commented Jun 29, 2015

Copy link
Copy Markdown
Contributor

This looks good, but we need a test...

@tbosch tbosch added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jun 29, 2015
@PatrickJS
PatrickJS force-pushed the patch-8 branch 2 times, most recently from ba6793c to 5446c6f Compare July 4, 2015 17:15
@PatrickJS

Copy link
Copy Markdown
Contributor Author

alright I rebased, refactored, and added a test.

@PatrickJS PatrickJS closed this Jul 5, 2015
@PatrickJS
PatrickJS deleted the patch-8 branch July 5, 2015 11:50
@PatrickJS
PatrickJS restored the patch-8 branch July 6, 2015 20:31
@PatrickJS PatrickJS reopened this Jul 6, 2015
@PatrickJS

Copy link
Copy Markdown
Contributor Author

oops I was cleaning up old branches and accidently deleted this

@tbosch tbosch added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jul 7, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By setting the type to ResponseTypes.Error on this ResponseOptions object, you're setting the default type to be Error, which is bad for the test, since we want to test that XHRBackend is smart enough to set the type as error based on the xhr erroring or getting a status that is considered an error.

@tbosch

tbosch commented Aug 18, 2015

Copy link
Copy Markdown
Contributor

@jeffbcross ping...

@jeffbcross

Copy link
Copy Markdown
Contributor

@gdi2290 finally scheduled this to be merged, thanks! https://github.com/angular/angular/tree/presubmit-jeffbcross-pr-2667

@PatrickJS
PatrickJS deleted the patch-8 branch August 19, 2015 01:53
@PatrickJS

Copy link
Copy Markdown
Contributor Author

thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: review The PR is still awaiting reviews from at least one requested reviewer cla: yes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants