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 7 times, most recently 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 89.84%. Comparing base (77e8d44) to head (e74c6fe).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61409      +/-   ##
==========================================
- Coverage   89.87%   89.84%   -0.03%     
==========================================
  Files         671      671              
  Lines      203178   203124      -54     
  Branches    39062    39040      -22     
==========================================
- Hits       182599   182503      -96     
- Misses      12926    12972      +46     
+ Partials     7653     7649       -4     
Files with missing lines Coverage Δ
lib/internal/encoding.js 100.00% <100.00%> (ø)

... and 29 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
@ChALkeR ChALkeR force-pushed the chalker/decoder/unify/1 branch from fb0ca32 to 1204484 Compare January 21, 2026 13:55
@ChALkeR ChALkeR force-pushed the chalker/decoder/unify/1 branch from 1204484 to e74c6fe Compare January 21, 2026 19:25
@ChALkeR
Copy link
Member Author

ChALkeR commented Jan 21, 2026

Rebased, no actual changes

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