Skip to content

Conversation

SeokhunEom
Copy link
Contributor

@SeokhunEom SeokhunEom commented Aug 23, 2025

Improve error handling in Buffer.concat. I replaced the generic ERR_INVALID_ARG_TYPE with the more specific ERR_INVALID_CONTAINER_ELEMENT_TYPE to provide a clearer error message. To make this change, I updated the test code in test-buffer-concat.js to check for the new error code and message format. And also added the ERR_INVALID_CONTAINER_ELEMENT_TYPE error type to lib/internal/errors.js and included documentation for it in doc/api/errors.md. This error type can be used for checking elements in continaers.

I think this is samver-major PR.

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. labels Aug 23, 2025
@SeokhunEom SeokhunEom force-pushed the buffer-concat-error branch 2 times, most recently from 18b7d69 to 553175d Compare August 23, 2025 11:22
Copy link

codecov bot commented Aug 23, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.85%. Comparing base (6fd67ec) to head (2811ce1).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59592      +/-   ##
==========================================
+ Coverage   89.82%   89.85%   +0.02%     
==========================================
  Files         666      667       +1     
  Lines      195337   196145     +808     
  Branches    38345    38512     +167     
==========================================
+ Hits       175463   176246     +783     
- Misses      12329    12364      +35     
+ Partials     7545     7535      -10     
Files with missing lines Coverage Δ
lib/buffer.js 100.00% <100.00%> (ø)
lib/internal/errors.js 97.61% <100.00%> (+0.10%) ⬆️

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

throw new ERR_INVALID_ARG_TYPE(
`list[${i}]`, ['Buffer', 'Uint8Array'], list[i]);
throw new ERR_INVALID_ARRAY_ELEMENT_TYPE(
'list', i, ['Buffer', 'Uint8Array'], list[i]);
Copy link
Member

Choose a reason for hiding this comment

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

I could imagine this error to be useful for other container types too (e.g. sets)... maybe the name shouldn't be specific to array? ERR_INVALID_CONTAINER_ELEMENT_TYPE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated to more general error type ERR_INVALID_CONTAINER_ELEMENT_TYPE.
Thank you for comment.

@jasnell
Copy link
Member

jasnell commented Aug 23, 2025

/cc @BridgeAR since it's your todo comment being addressed here :-)

- Replace ERR_INVALID_ARG_TYPE with ERR_INVALID_CONTAINER_ELEMENT_TYPE
  for clarity in error messages
- Update test-buffer-concat.js with new error code and message format
- Add new error ERR_INVALID_CONTAINER_ELEMENT_TYPE
  in lib/internal/errors.js
- Add document for ERR_INVALID_CONTAINER_ELEMENT_TYPE
  in doc/api/errors.md
@SeokhunEom SeokhunEom force-pushed the buffer-concat-error branch from 553175d to 2811ce1 Compare August 23, 2025 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants