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): Set content-length only if body is present #638

Merged
merged 1 commit into from
Nov 21, 2024
Merged

fix(http): Set content-length only if body is present #638

merged 1 commit into from
Nov 21, 2024

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Nov 21, 2024

No description provided.

Copy link

changeset-bot bot commented Nov 21, 2024

🦋 Changeset detected

Latest commit: 0d304d2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@opennextjs/aws Patch
app-pages-router Patch
app-router Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Nov 21, 2024

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 22.17% 1659 / 7482
🔵 Statements 22.17% 1659 / 7482
🔵 Functions 56.57% 99 / 175
🔵 Branches 70.4% 414 / 588
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/open-next/src/http/request.ts 100% 100% 66.66% 100%
Generated in workflow #765 for commit 0d304d2 by the Vitest Coverage Report Action

Copy link

pkg-pr-new bot commented Nov 21, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/aws@638

commit: 0d304d2

body,
remoteAddress,
}: {
constructor(request: {
Copy link
Contributor Author

@vicb vicb Nov 21, 2024

Choose a reason for hiding this comment

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

I had to name the parameter because of a formatting issue.

    // Set the content length when there is a body.
    // See https://httpwg.org/specs/rfc9110.html#field.content-length
    if (body) {
      headers["content-length"] ??= String(Buffer.byteLength(body));
    }

When saving the file with the previous structure the line if (body) { would be deleted otherwise 🤨
This would not happen if the comments are moved after the if.
Weird bug but still happens after ESLint and Biome extensions are de-activated in VSCode.

🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, very weird bug indeed

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably worth to add a comment explaining this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well... amending the PR to revert the change worked.

To prove I'm not completely 🤪, here is a video when I start from the original file:

Screen.Recording.2024-11-21.at.14.43.09.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

To prove I'm not completely 🤪, here is a video when I start from the original file:

does it happen if you use /* ... */?

Copy link
Contributor Author

@vicb vicb Nov 21, 2024

Choose a reason for hiding this comment

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

Yep, tried that as well as a few different (potential) workarounds and it results in the same weird bug.

Copy link
Contributor

@conico974 conico974 left a comment

Choose a reason for hiding this comment

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

Just a little comment, otherwise LGTM Thanks

body,
remoteAddress,
}: {
constructor(request: {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably worth to add a comment explaining this

@vicb vicb merged commit fe600ac into main Nov 21, 2024
3 checks passed
@vicb vicb deleted the request branch November 21, 2024 13:49
@github-actions github-actions bot mentioned this pull request Nov 19, 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.

3 participants