Conversation
|
Review requested:
|
11b6878 to
329d0a0
Compare
0ec4ccc to
bd305a8
Compare
|
@nodejs/http @mcollina |
|
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
left a comment
There was a problem hiding this comment.
Yay! thanks for doing this! 🫶
(with this all pesky _modules will be runtime deprecated! 💪)
Most of them seem to be just forks of Node.js, rather than actually using these undocumented modules. |
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 Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #58535 +/- ##
========================================
Coverage 89.62% 89.63%
========================================
Files 706 712 +6
Lines 219136 219384 +248
Branches 41987 41991 +4
========================================
+ Hits 196404 196640 +236
- Misses 14611 14621 +10
- Partials 8121 8123 +2
🚀 New features to boost your workflow:
|
|
@nodejs/http what are your recommendations? |
Ref nodejs#58535 Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Ref nodejs#58535 Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Ref nodejs#58535 Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Ref #58535 Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com> PR-URL: #59293 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
|
cc: @nodejs/tsc @nodejs/http |
8d9177f to
731360d
Compare
gurgunday
left a comment
There was a problem hiding this comment.
lgtm
I think we can now move forward with this
|
@nodejs/tsc could we take a look at this? I can rebase it again, but the PR always ends up stuck without a review. |
|
rebase and add the fast track label |
|
tbh it's not a rebase that is needed but moving the new files |
731360d to
ec6cb5c
Compare
|
Fast-track has been requested by @bjohansebas. Please 👍 to approve. |
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
ec6cb5c to
6177039
Compare
|
Fast-track has been requested by @bjohansebas. Please 👍 to approve. |
Failed to start CI- Validating Jenkins credentials ✔ Jenkins credentials valid - Querying data for job/node-test-pull-request/71184/ [SyntaxError: Unexpected token '<', ..." https://github.com/nodejs/node/actions/runs/24639573120 |
close #58534