Skip to content

util: add internal assignFunctionName() function - #57916

Closed
LiviaMedeiros wants to merge 3 commits into
nodejs:mainfrom
LiviaMedeiros:util-internal-assign-function-name
Closed

util: add internal assignFunctionName() function#57916
LiviaMedeiros wants to merge 3 commits into
nodejs:mainfrom
LiviaMedeiros:util-internal-assign-function-name

Conversation

@LiviaMedeiros

Copy link
Copy Markdown
Member

Extracted from: #57901

We have a lot of functions exposed to userspace or mentioned in stack traces that are anonymous or have something inherited like value as name; because it's not convenient to define it with name, or because the correct name is already taken by another variable, or because the name comes from a Symbol, or because of false positives from linter.

This helper function will simplify giving such functions a correct name.

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Apr 17, 2025
@BridgeAR

Copy link
Copy Markdown
Member

I think it's best to introduce the helper in the other PR, since it is used that way.

@LiviaMedeiros

Copy link
Copy Markdown
MemberAuthor

I extracted this so it could be used for other namings (e.g. for some [Symbol(nodejs.util.promisify.custom)] functions), since it's not related to the other PR, doesn't have to depend on it, and would simplify the process (especially for backports and cherry-picking later). But it's feasible either way.

@BridgeAR

Copy link
Copy Markdown
Member

I would definitely like adding usages right away. It would otherwise feel weird to me to add a utility function for internal usage only. We could of course split it into two commits in the same PR.

@codecov

codecovBot commented Apr 17, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.27%. Comparing base (609df89) to head (5292de9).
Report is 127 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #57916 +/- ##
==========================================
- Coverage 90.27% 90.27% -0.01% 
==========================================
Files 630 630 Lines 186112 186145 +33 Branches 36464 36471 +7 ==========================================
+ Hits 168013 168042 +29 + Misses 10981 10970 -11 - Partials 7118 7133 +15 
Files with missing linesCoverage Δ
lib/child_process.js97.75% <100.00%> (+<0.01%)⬆️
lib/internal/http2/core.js95.63% <100.00%> (+<0.01%)⬆️
lib/internal/util.js96.30% <100.00%> (+0.56%)⬆️

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

@LiviaMedeirosLiviaMedeiros added lib / src Issues and PRs related to general changes in the lib or src directory. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Apr 18, 2025
@LiviaMedeiros
LiviaMedeirosforce-pushed the util-internal-assign-function-name branch from fd20019 to 18b89d3CompareApril 18, 2025 06:39
Comment threadlib/internal/util.js Outdated
Comment threadlib/child_process.js Outdated
Comment threadlib/internal/util.js Outdated
@LiviaMedeirosLiviaMedeiros self-assigned this Apr 19, 2025
Comment threadlib/internal/util.js Outdated

@H4adH4ad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be marked as minor since we change the behavior a little bit (although it's not disrupting)?

@aduh95

Copy link
Copy Markdown
Contributor

If we change the semverness of this PR, we should definitely split it in several PRs to simplify work for releasers

@LiviaMedeiros

Copy link
Copy Markdown
MemberAuthor

Should this be marked as minor since we change the behavior a little bit (although it's not disrupting)?

I'm okay with semverness change if needed, but not sure it can be semver-minor, since it doesn't add any userland-facing features. IMHO the helper function by itself is semver-patch, and the other changes are either semver-patch fixes or semver-major (potentially) breaking fixes.

Comment threadlib/internal/http2/core.js Outdated
@LiviaMedeiros
LiviaMedeirosforce-pushed the util-internal-assign-function-name branch from 83ced41 to 22c521bCompareApril 19, 2025 17:35
@aduh95aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 19, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@LiviaMedeirosLiviaMedeiros removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 20, 2025
@LiviaMedeiros
LiviaMedeirosforce-pushed the util-internal-assign-function-name branch from ee4de6c to 32d29feCompareApril 20, 2025 06:58
@nodejs-github-bot

This comment was marked as outdated.

@LiviaMedeirosLiviaMedeiros added the commit-queue Add this label to land a pull request using GitHub Actions. label May 4, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 4, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 9acf6af...a44ccac

nodejs-github-bot pushed a commit that referenced this pull request May 4, 2025
PR-URL: #57916
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request May 4, 2025
PR-URL: #57916
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request May 4, 2025
PR-URL: #57916
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 16, 2025
PR-URL: #57916
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 16, 2025
PR-URL: #57916
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 16, 2025
PR-URL: #57916
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Jun 10, 2025
PR-URL: #57916
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Jun 10, 2025
PR-URL: #57916
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Jun 10, 2025
PR-URL: #57916
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Jun 26, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 27, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 27, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 28, 2025
* chore: bump node in DEPS to v22.17.0
* build: use //third_party/simdutf by default in GN
nodejs/node#58115
* chore: adjust crypto specs:
- nodejs/node#58117
- nodejs/node#58387
* deps: update libuv to 1.51.0
nodejs/node#58124
* test: fix test-buffer-tostring-range on allocation failure
nodejs/node#58416
* build: use FILE_OFFSET_BITS=64 esp. on 32-bit arch
nodejs/node#58090
* build: use //third_party/simdutf by default in GN
nodejs/node#58115
* inspector: add protocol method Network.dataReceived
nodejs/node#58001
* test: force slow JSON.stringify path for overflow
nodejs/node#58181
* chore: fixup patch indices
* 6049967: Remove protocol::Maybe and roll inspector_protocol
https://chromium-review.googlesource.com/c/chromium/src/+/6049967
* chore: fixup crypto test patch
* src: fix module buffer allocation
nodejs/node#57738
* crypto: expose process.features.openssl_is_boringssl
nodejs/node#58387
* util: add internal assignFunctionName() function
nodejs/node#57916
* build: fix pointer compression builds
nodejs/node#58171
* chore: put back config options
* fixup! deps: update libuv to 1.51.0
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025
* chore: bump node in DEPS to v22.17.0
* build: use //third_party/simdutf by default in GN
nodejs/node#58115
* chore: adjust crypto specs:
- nodejs/node#58117
- nodejs/node#58387
* deps: update libuv to 1.51.0
nodejs/node#58124
* test: fix test-buffer-tostring-range on allocation failure
nodejs/node#58416
* build: use FILE_OFFSET_BITS=64 esp. on 32-bit arch
nodejs/node#58090
* build: use //third_party/simdutf by default in GN
nodejs/node#58115
* inspector: add protocol method Network.dataReceived
nodejs/node#58001
* test: force slow JSON.stringify path for overflow
nodejs/node#58181
* chore: fixup patch indices
* 6049967: Remove protocol::Maybe and roll inspector_protocol
https://chromium-review.googlesource.com/c/chromium/src/+/6049967
* chore: fixup crypto test patch
* src: fix module buffer allocation
nodejs/node#57738
* crypto: expose process.features.openssl_is_boringssl
nodejs/node#58387
* util: add internal assignFunctionName() function
nodejs/node#57916
* build: fix pointer compression builds
nodejs/node#58171
* chore: put back config options
* fixup! deps: update libuv to 1.51.0
* chore: update patches
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: patchup[bot] <73610968+patchup[bot]@users.noreply.github.com>
codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025
* chore: bump node in DEPS to v22.17.0
* build: use //third_party/simdutf by default in GN
nodejs/node#58115
* chore: adjust crypto specs:
- nodejs/node#58117
- nodejs/node#58387
* deps: update libuv to 1.51.0
nodejs/node#58124
* test: fix test-buffer-tostring-range on allocation failure
nodejs/node#58416
* build: use FILE_OFFSET_BITS=64 esp. on 32-bit arch
nodejs/node#58090
* build: use //third_party/simdutf by default in GN
nodejs/node#58115
* inspector: add protocol method Network.dataReceived
nodejs/node#58001
* test: force slow JSON.stringify path for overflow
nodejs/node#58181
* chore: fixup patch indices
* 6049967: Remove protocol::Maybe and roll inspector_protocol
https://chromium-review.googlesource.com/c/chromium/src/+/6049967
* chore: fixup crypto test patch
* src: fix module buffer allocation
nodejs/node#57738
* crypto: expose process.features.openssl_is_boringssl
nodejs/node#58387
* util: add internal assignFunctionName() function
nodejs/node#57916
* build: fix pointer compression builds
nodejs/node#58171
* chore: put back config options
* fixup! deps: update libuv to 1.51.0
* chore: update patches
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: patchup[bot] <73610968+patchup[bot]@users.noreply.github.com>
codebytere added a commit to electron/electron that referenced this pull request Jul 2, 2025
codebytere added a commit to electron/electron that referenced this pull request Jul 2, 2025
* chore: bump node in DEPS to v22.17.0
* build: use //third_party/simdutf by default in GN
nodejs/node#58115
* chore: adjust crypto specs:
- nodejs/node#58117
- nodejs/node#58387
* deps: update libuv to 1.51.0
nodejs/node#58124
* test: fix test-buffer-tostring-range on allocation failure
nodejs/node#58416
* build: use FILE_OFFSET_BITS=64 esp. on 32-bit arch
nodejs/node#58090
* build: use //third_party/simdutf by default in GN
nodejs/node#58115
* inspector: add protocol method Network.dataReceived
nodejs/node#58001
* chore: fixup patch indices
* 6049967: Remove protocol::Maybe and roll inspector_protocol
https://chromium-review.googlesource.com/c/chromium/src/+/6049967
* chore: fixup crypto test patch
* src: fix module buffer allocation
nodejs/node#57738
* crypto: expose process.features.openssl_is_boringssl
nodejs/node#58387
* util: add internal assignFunctionName() function
nodejs/node#57916
* build: fix pointer compression builds
nodejs/node#58171
* chore: put back config options
* fixup! deps: update libuv to 1.51.0
* chore: update patches
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: patchup[bot] <73610968+patchup[bot]@users.noreply.github.com>
kigh-ota pushed a commit to kigh-ota/electron that referenced this pull request Sep 30, 2025
* chore: bump node in DEPS to v22.17.0
* build: use //third_party/simdutf by default in GN
nodejs/node#58115
* chore: adjust crypto specs:
- nodejs/node#58117
- nodejs/node#58387
* deps: update libuv to 1.51.0
nodejs/node#58124
* test: fix test-buffer-tostring-range on allocation failure
nodejs/node#58416
* build: use FILE_OFFSET_BITS=64 esp. on 32-bit arch
nodejs/node#58090
* build: use //third_party/simdutf by default in GN
nodejs/node#58115
* inspector: add protocol method Network.dataReceived
nodejs/node#58001
* test: force slow JSON.stringify path for overflow
nodejs/node#58181
* chore: fixup patch indices
* 6049967: Remove protocol::Maybe and roll inspector_protocol
https://chromium-review.googlesource.com/c/chromium/src/+/6049967
* chore: fixup crypto test patch
* src: fix module buffer allocation
nodejs/node#57738
* crypto: expose process.features.openssl_is_boringssl
nodejs/node#58387
* util: add internal assignFunctionName() function
nodejs/node#57916
* build: fix pointer compression builds
nodejs/node#58171
* chore: put back config options
* fixup! deps: update libuv to 1.51.0
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-rebaseAdd this label to allow the Commit Queue to land a PR in several commits.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.utilIssues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@LiviaMedeiros@BridgeAR@aduh95@nodejs-github-bot@jasnell@H4ad