Skip to content

fix(http): Include HTTP status code and headers when HTTP requests errored in httpResource#60802

Closed
wartab wants to merge 1 commit intoangular:mainfrom
wartab:http-resource-status-headers
Closed

fix(http): Include HTTP status code and headers when HTTP requests errored in httpResource#60802
wartab wants to merge 1 commit intoangular:mainfrom
wartab:http-resource-status-headers

Conversation

@wartab
Copy link
Copy Markdown
Contributor

@wartab wartab commented Apr 9, 2025

Currently the HTTP status code and headers are only included if the request succeeded. Given status codes convey more information in case of a request error vs. success, this makes it more useful than inspecting what is contained in .error().

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.dev application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I'm not sure if the current behaviour is actually intended, but I don't really see why statusCode would exist if it was not meant to contain something else than 200 (or 204, but you get my point).

@pullapprove pullapprove bot requested a review from crisbeto April 9, 2025 13:16
@angular-robot angular-robot bot added the area: common/http Issues related to HTTP and HTTP Client label Apr 9, 2025
@ngbot ngbot bot added this to the Backlog milestone Apr 9, 2025
@JeanMeche JeanMeche removed the request for review from crisbeto April 9, 2025 15:26
JeanMeche
JeanMeche approved these changes Apr 9, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@JeanMeche JeanMeche left a comment

Choose a reason for hiding this comment

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

It looks like it broke some tests, can you have a look at it. Thank you.

@JeanMeche JeanMeche added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Apr 9, 2025
@wartab wartab force-pushed the http-resource-status-headers branch from c21e07f to 9a46c98 Compare April 9, 2025 15:41
@angular-robot angular-robot bot requested a review from JeanMeche April 9, 2025 15:41
@wartab
Copy link
Copy Markdown
Contributor Author

wartab commented Apr 9, 2025

Yeah, just saw it, I tried to run them on my computer, but there were a lot of errors that were related to not being able to run the chrome sandboxes that crashed with SEGFAULTs.

@JeanMeche
Copy link
Copy Markdown
Member

JeanMeche commented Apr 9, 2025

You should be able to run the node only test with yarn bazel //packages/common/http/test:test

@wartab
fix(http): Include HTTP status code and headers when HTTP requests er…
424ea34 
…rored in `httpResource`

Currently the HTTP status code and headers are only included if the request succeeded. Given status codes convey more information in case of a request error vs. success, this makes it more useful than inspecting what is contained in `.error()`.
@wartab wartab force-pushed the http-resource-status-headers branch from 9a46c98 to 424ea34 Compare April 9, 2025 15:52
@wartab
Copy link
Copy Markdown
Contributor Author

wartab commented Apr 9, 2025 •
edited
Loading

Ah thanks, that is faster as well :') I ran it on //packages/common

I just fixed the tests.

@JeanMeche JeanMeche added action: merge The PR is ready for merge by the caretaker 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 Apr 9, 2025
atscott pushed a commit that referenced this pull request Apr 9, 2025
@wartab @atscott
fix(http): Include HTTP status code and headers when HTTP requests er…
d39e09d 
…rored in `httpResource` (#60802)

Currently the HTTP status code and headers are only included if the request succeeded. Given status codes convey more information in case of a request error vs. success, this makes it more useful than inspecting what is contained in `.error()`.

PR Close #60802
@atscott
Copy link
Copy Markdown
Contributor

atscott commented Apr 9, 2025

This PR was merged into the repository by commit 9f31947.

The changes were merged into the following branches: main, 19.2.x

@atscott atscott closed this in 9f31947 Apr 9, 2025
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented May 10, 2025

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes

@JeanMeche JeanMeche JeanMeche approved these changes

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: common/http Issues related to HTTP and HTTP Client target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

4 participants

@wartab @JeanMeche @atscott @pkozlowski-opensource