Skip to content

fix(core): refine HttpError.status property type#2890

Merged
marvinhagemeister merged 16 commits into
freshframework:mainfrom
iuioiua:http-error-refine
Jun 16, 2025
Merged

fix(core): refine HttpError.status property type#2890
marvinhagemeister merged 16 commits into
freshframework:mainfrom
iuioiua:http-error-refine

Conversation

@iuioiua

@iuioiua iuioiua commented May 6, 2025

Copy link
Copy Markdown
Contributor

As this is property only ever has an error HTTP status code (>400).

@marvinhagemeister marvinhagemeister left a comment

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.

The assumption that only an instance of HttpError is thrown is not correct. There is no guarantee for that, see:

app.use(() => {
  throw "foo"
});

@iuioiua

iuioiua commented May 12, 2025

Copy link
Copy Markdown
Contributor Author

Ah, yes 🤦🏾‍♂️ I did at least assume that an Error is thrown.

@iuioiua iuioiua requested a review from marvinhagemeister May 12, 2025 07:48
@iuioiua iuioiua marked this pull request as draft May 23, 2025 09:17
@iuioiua iuioiua marked this pull request as ready for review May 24, 2025 04:00
@iuioiua

iuioiua commented May 27, 2025

Copy link
Copy Markdown
Contributor Author

Gentle ping @marvinhagemeister

@marvinhagemeister marvinhagemeister left a comment

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.

LGTM

@marvinhagemeister marvinhagemeister merged commit c3c7935 into freshframework:main Jun 16, 2025
7 checks passed
@iuioiua iuioiua deleted the http-error-refine branch June 16, 2025 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants