Skip to content

Conversation

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Jan 17, 2026

Better reviewed with whitespace ignored

  1. Fix TextDecoder defaults to stream: true instead of false with null options #61411
  2. Add support for fatal UTF-8 decoder in no-ICU version unless streaming is used
  3. add utf-8 fast path to no-ICU version
  4. reduce duplication of TextDecoder source
  5. normalize how this[kHandle] was not set in no-ICU version
  6. The refactor changes the encoding reported in the no-ICU version when encoding is recognized but unsupported: now the actual resolved encoding name is reported as unsupported when previously the original input string was reported.
    I don't think this is major, and reporting the actual encoding name is better in that case.

This will allow further fixes and common fast path for UTF-16 / fixed impl for UTF-16 as string_decoder doesn't implement UTF-16 per spec.

UTF-16 is still broken in no-ICU version here, this PR does not address that.
Tracking: #61041

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 17, 2026
@ChALkeR ChALkeR changed the title Chalker/decoder/unify/1 lib: unify ICU and no-ICU TextDecoder Jan 17, 2026
@ChALkeR ChALkeR force-pushed the chalker/decoder/unify/1 branch 2 times, most recently from 367908c to 5a2317f Compare January 17, 2026 14:24
@ChALkeR ChALkeR force-pushed the chalker/decoder/unify/1 branch 5 times, most recently from 4d6ff8f to b2e1ece Compare January 17, 2026 23:54
@ChALkeR ChALkeR marked this pull request as ready for review January 17, 2026 23:55
@ChALkeR ChALkeR force-pushed the chalker/decoder/unify/1 branch 6 times, most recently from 69e9173 to adb2410 Compare January 18, 2026 00:20
@ChALkeR ChALkeR force-pushed the chalker/decoder/unify/1 branch from adb2410 to fb0ca32 Compare January 18, 2026 00:29
@codecov
Copy link

codecov bot commented Jan 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.50%. Comparing base (955d347) to head (fb0ca32).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61409      +/-   ##
==========================================
- Coverage   88.52%   88.50%   -0.02%     
==========================================
  Files         704      704              
  Lines      208802   208830      +28     
  Branches    40318    40311       -7     
==========================================
- Hits       184842   184833       -9     
- Misses      15947    15968      +21     
- Partials     8013     8029      +16     
Files with missing lines Coverage Δ
lib/internal/encoding.js 100.00% <100.00%> (ø)

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

Comment on lines +420 to +425
let StringDecoder;
function lazyStringDecoder() {
if (StringDecoder === undefined)
({ StringDecoder } = require('string_decoder'));
return StringDecoder;
}
Copy link
Member

Choose a reason for hiding this comment

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

There's a lazy utility in the internal utils.

Copy link
Member Author

@ChALkeR ChALkeR Jan 20, 2026

Choose a reason for hiding this comment

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

@avivkeller this is not new code, it's just moved to an outer scope
Ideally all that should go away with follow-up fixes, string_decoder path is invalid anyway
I don't think it's worth refactoring it further

@ChALkeR ChALkeR requested a review from avivkeller January 20, 2026 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TextDecoder defaults to stream: true instead of false with null options

3 participants