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

fix HTTP 301/302 not work with AwsStreamHandler #204

Closed
wants to merge 1 commit into from

Conversation

stackia
Copy link
Contributor

@stackia stackia commented Feb 22, 2024

Description of change

300 Multiple Choices
301 Moved Permanently
302 Found
303 See Other
304 Not Modified
307 Temporary Redirect
308 Permanent Redirect

I believe these HTTP code should have no body as well.

Or at least make 301/302 in the list, otherwise we cannot send 301/302 redirect with AwsStreamHandler.

Copy link
Owner

@H4ad H4ad left a comment

Choose a reason for hiding this comment

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

Did you had issue while integrating with AWS Stream Response with these status? Because from what I remember, in the past when I tested, I didn't had issues.

Also, can you please change your commit message to be: fix(aws): HTTP 301/302 not work with AwsStreamHandler

@@ -301,7 +301,7 @@ export class AwsStreamHandler<TApp> extends BaseHandler<
// so I have this thing just to fix this issue
// ref: https://stackoverflow.com/a/37303151
const isHundreadStatus = status >= 100 && status < 200;
const isNoContentStatus = status === 304 || status === 204;
const isNoContentStatus = status >= 300 && status < 400;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const isNoContentStatus = status >= 300 && status < 400;
const isNoContentStatus = (status >= 300 && status < 400) || status === 204;

@stackia
Copy link
Contributor Author

stackia commented Feb 28, 2024

I created another PR to address this issue.

#206

@stackia stackia closed this Feb 28, 2024
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