lib: deprecate _http_* #58535

bjohansebas · 2025-05-31T19:21:56Z

nodejs-github-bot · 2025-05-31T19:22:01Z

Review requested:

@nodejs/http
@nodejs/net

jasnell · 2025-05-31T23:19:41Z

benchmark/http/check_is_http_token.js

@@ -1,6 +1,7 @@
 'use strict';

 const common = require('../common.js');
+// TODO: benchmark has access to internal modules?
 const _checkIsHttpToken = require('_http_common')._checkIsHttpToken;


These aren't really considered internal modules, fwiw

But that property is not under the http module, so when _http_common stops being a module and becomes internal-only, the benchmark won’t work. That’s why I was asking if it's possible to use internals in the benchmarks.

yes I imagine that the require was supposed to be updated to internal/http/common?

My concern would be whether or not anyone in userland could be using this, as unlikely as that may be. Should we export this through the regular node:http API to provide an alternative path to them?

jasnell · 2025-05-31T23:22:01Z

@nodejs/http @mcollina

jasnell · 2025-05-31T23:23:48Z

This one might need a Documentation-only deprecation to start. It's likely going to be too disruptive to go straight to a Runtime deprecation. https://github.com/search?type=code&q=%22require%28%27_http%22

dario-piotrowicz

Yay! thanks for doing this! 🫶

(with this all pesky _modules will be runtime deprecated! 💪)

test/parallel/test-warn-http-server-deprecation.js

dario-piotrowicz · 2025-05-31T23:23:56Z

test/parallel/test-https-server-close-destroy-timeout.js

@@ -1,3 +1,4 @@
+// Flags: --expose-internals --no-warnings


why the addition of --no-warning? here and in the other tests

I only saw it in other tests, I thought it was necessary.

I checked this and I must say that I find it very weird.... I went and checked existing--no-warning I cannot really understand why they are there, since the test script suppresses outputs anyways, I totally cannot see any difference when I try to remove them... 🤷

dario-piotrowicz · 2025-05-31T23:27:02Z

benchmark/http/check_is_http_token.js

@@ -1,6 +1,7 @@
 'use strict';

 const common = require('../common.js');
+// TODO: benchmark has access to internal modules?
 const _checkIsHttpToken = require('_http_common')._checkIsHttpToken;


yes I imagine that the require was supposed to be updated to internal/http/common?

dario-piotrowicz · 2025-05-31T23:27:51Z

benchmark/http/check_invalid_header_char.js

@@ -1,6 +1,7 @@
 'use strict';

 const common = require('../common.js');
+// TODO: benchmark has access to internal modules?


should the require be updated to internal/http/common? 🙂

yes, but I’m not really sure if the benchmark has access to the internals or not :)

ah no.... the benchmarks, as far as I understand it, should be runnable with any build of node, so yeah that's a good point, I don't think it's correct to have a benchmark access internal modules

bjohansebas · 2025-05-31T23:31:09Z

This one might need a Documentation-only deprecation to start. It's likely going to be too disruptive to go straight to a Runtime deprecation. https://github.com/search?type=code&q=%22require%28%27_http%22

Most of them seem to be just forks of Node.js, rather than actually using these undocumented modules.

Co-authored-by: Dario Piotrowicz <[email protected]>

jasnell · 2025-05-31T23:45:08Z

Most of them seem to be just forks of Node.js....

I wish that were the case. While there are a good number of forks in those search results, it's plain to see that there are a non-trivial number of other projects requiring "_http_common" and friends. We need to assess just how disruptive this will be... but don't get me wrong, I am in favor of deprecated these but it might need to be a slower path. I'd like @nodejs/http folks to weigh in.

codecov · 2025-06-01T00:29:07Z

Codecov Report

Attention: Patch coverage is 97.49006% with 120 lines in your changes missing coverage. Please review.

Project coverage is 90.25%. Comparing base (f497881) to head (2103d6f).
Report is 105 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http/outgoing.js	95.76%	48 Missing and 3 partials ⚠️
lib/internal/http/server.js	97.07%	33 Missing and 3 partials ⚠️
lib/internal/http/client.js	97.99%	20 Missing ⚠️
lib/internal/http/agent.js	98.17%	10 Missing ⚠️
lib/internal/http/incoming.js	99.33%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58535      +/-   ##
==========================================
+ Coverage   90.23%   90.25%   +0.01%     
==========================================
  Files         635      641       +6     
  Lines      187580   187832     +252     
  Branches    36860    36862       +2     
==========================================
+ Hits       169265   169520     +255     
+ Misses      11101    11095       -6     
- Partials     7214     7217       +3

Files with missing lines	Coverage Δ
lib/_http_agent.js	`100.00% <100.00%> (+1.82%)`	⬆️
lib/_http_client.js	`100.00% <100.00%> (+2.00%)`	⬆️
lib/_http_common.js	`100.00% <100.00%> (ø)`
lib/_http_incoming.js	`100.00% <100.00%> (+0.66%)`	⬆️
lib/_http_outgoing.js	`100.00% <100.00%> (+4.23%)`	⬆️
lib/_http_server.js	`100.00% <100.00%> (+3.16%)`	⬆️
lib/http.js	`100.00% <100.00%> (ø)`
lib/https.js	`99.30% <100.00%> (ø)`
lib/internal/child_process.js	`95.06% <100.00%> (ø)`
lib/internal/http/common.js	`100.00% <100.00%> (ø)`
... and 8 more

... and 25 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.

dario-piotrowicz · 2025-06-01T21:45:29Z

benchmark/http/check_invalid_header_char.js

@@ -1,6 +1,7 @@
 'use strict';

 const common = require('../common.js');
+// TODO: benchmark has access to internal modules?


ah no.... the benchmarks, as far as I understand it, should be runnable with any build of node, so yeah that's a good point, I don't think it's correct to have a benchmark access internal modules

dario-piotrowicz · 2025-06-01T21:47:18Z

lib/_http_common.js

+  _checkIsHttpToken,
+  chunkExpression,
+  continueExpression,
+  CRLF, // TODO: Deprecate this.


Suggested change

CRLF, // TODO: Deprecate this.

CRLF,

no need to have a deprecate todo inside a deprecated module right? (and I don't think this is re-exported by the public http module, right?)

dario-piotrowicz · 2025-06-01T22:21:48Z

test/parallel/test-https-server-close-destroy-timeout.js

@@ -1,3 +1,4 @@
+// Flags: --expose-internals --no-warnings


I checked this and I must say that I find it very weird.... I went and checked existing--no-warning I cannot really understand why they are there, since the test script suppresses outputs anyways, I totally cannot see any difference when I try to remove them... 🤷

bjohansebas · 2025-06-14T18:03:35Z

@nodejs/http what are your recommendations?

nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels May 31, 2025

bjohansebas added the wip Issues and PRs that are still a work in progress. label May 31, 2025

bjohansebas force-pushed the http_internals branch 2 times, most recently from 11b6878 to 329d0a0 Compare May 31, 2025 20:17

bjohansebas added semver-major PRs that contain breaking changes and should be released in the next major version. deprecations Issues and PRs related to deprecations. labels May 31, 2025

bjohansebas marked this pull request as ready for review May 31, 2025 21:29

bjohansebas requested review from mcollina, jasnell and dario-piotrowicz May 31, 2025 21:29

bjohansebas removed the wip Issues and PRs that are still a work in progress. label May 31, 2025

bjohansebas added 3 commits May 31, 2025 22:32

lib: deprecate _http_*

e6335e0

test: update internal module references and add flags for tests

e6a2193

http,http2,https: use http internals

cdce873

bjohansebas force-pushed the http_internals branch from 674ca6f to 0ec4ccc Compare May 31, 2025 22:36

bjohansebas added 2 commits May 31, 2025 23:16

test: add test for warnings

5b223df

benchmack: no use internals

bd305a8

bjohansebas force-pushed the http_internals branch from 0ec4ccc to bd305a8 Compare May 31, 2025 23:17

jasnell reviewed May 31, 2025

View reviewed changes

dario-piotrowicz reviewed May 31, 2025

View reviewed changes

Update test/parallel/test-warn-http-server-deprecation.js

2103d6f

Co-authored-by: Dario Piotrowicz <[email protected]>

dario-piotrowicz reviewed Jun 1, 2025

View reviewed changes

Uh oh!

lib: deprecate _http_* #58535

Are you sure you want to change the base?

lib: deprecate _http_* #58535

Uh oh!

Conversation

bjohansebas commented May 31, 2025

Uh oh!

nodejs-github-bot commented May 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented May 31, 2025

Uh oh!

jasnell commented May 31, 2025

Uh oh!

dario-piotrowicz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjohansebas commented May 31, 2025

Uh oh!

jasnell commented May 31, 2025

Uh oh!

codecov bot commented Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dario-piotrowicz Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjohansebas commented Jun 14, 2025

Uh oh!

Uh oh!

dario-piotrowicz Jun 1, 2025 •

edited

Loading

codecov bot commented Jun 1, 2025 •

edited

Loading

dario-piotrowicz Jun 1, 2025 •

edited

Loading