Skip to content

http, benchmark: remove CRLF variables #59466

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

wlgh1553
Copy link
Contributor

This PR replaces remaining usages of the CRLF variable with the string literal\r\n and removes theCRLF export from _http_common.js.

Background

In #40101, it was discussed that using the string literal \r\n is clearer and more explicit than using a CRLF variable. Based on this consensus, this PR replaces all remaining usages of the CRLF variable with the \r\n literal.

About removing the export

Previously, there was a TODO comment not to remove the CRLF export from _http_common.js because the module was considered "semi-public" and should not have breaking changes without proper deprecation (discussion).

However, as of #59293, the _http_common.js module (along with other node:_http_* modules) is now officially deprecated and considered internal-only:

Given this, I considered it appropriate to remove the CRLF export.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net
  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Aug 14, 2025
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link

codecov bot commented Aug 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.88%. Comparing base (f7a2ba7) to head (eff20f7).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59466      +/-   ##
==========================================
- Coverage   89.88%   89.88%   -0.01%     
==========================================
  Files         656      656              
  Lines      192963   192963              
  Branches    37845    37850       +5     
==========================================
- Hits       173448   173443       -5     
+ Misses      12049    12046       -3     
- Partials     7466     7474       +8     

see 42 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pimterry pimterry added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 14, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2025
@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member

About removing the export

Previously, there was a TODO comment not to remove the CRLF export from _http_common.js because the module was considered "semi-public" and should not have breaking changes without proper deprecation (discussion).

However, as of #59293, the _http_common.js module (along with other node:_http_* modules) is now officially deprecated and considered internal-only:

Given this, I considered it appropriate to remove the CRLF export.

The node:_http_* modules are only documentation deprecated. We can't remove the functionality until End-of-Life deprecation.

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

#59466 (comment)

We should not remove the export until End-of-Life deprecation.

@richardlau richardlau removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 14, 2025
@wlgh1553
Copy link
Contributor Author

#59466 (comment)

We should not remove the export until End-of-Life deprecation.

Thanks for the review! Based on your feedback, I've added a commit to restore the CRLF export. eff20f7

@richardlau richardlau dismissed their stale review August 15, 2025 00:24

Concern has been addressed

@richardlau richardlau added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 15, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 15, 2025
@nodejs-github-bot
Copy link
Collaborator

@wlgh1553 wlgh1553 force-pushed the remove-crlf-variable branch from eff20f7 to 080e9d3 Compare August 17, 2025 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants