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.78%. Comparing base (77e8d44) to head (54e7ba2).
⚠️ Report is 47 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61409      +/-   ##
==========================================
- Coverage   89.87%   89.78%   -0.09%     
==========================================
  Files         671      672       +1     
  Lines      203178   203876     +698     
  Branches    39062    39186     +124     
==========================================
+ Hits       182599   183051     +452     
- Misses      12926    13142     +216     
- Partials     7653     7683      +30     
Files with missing lines Coverage Δ
lib/internal/encoding.js 100.00% <100.00%> (ø)

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

Copy link
Member

Choose a reason for hiding this comment

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

I still think we should use the new lazy module. Regardless of this being new code or not, git sees it as a new code.

Copy link
Member Author

@ChALkeR ChALkeR Jan 23, 2026

Choose a reason for hiding this comment

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

@anonrig Next two prs will remove this anyway. This is going in iterative steps, the intention of this PR is to make rewrite easier, not to be that rewrite. I don't think it should be blocked on rewriting a code block that wasn't introduced here and is gonna be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

#61549 removes string_decoder usage for UTF-8

For UTF-16 it's broken currently and will be refactored in a similar way in once #61549 lands, then string_decoder import will be just removed here

@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

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

@gurgunday gurgunday added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 25, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 25, 2026
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member

The withoutintl test failure looks related.
https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu2404_sharedlibs_withoutintl_x64/54468/console

19:12:10 not ok 3707 parallel/test-whatwg-encoding-custom-textdecoder
19:12:10   ---
19:12:10   duration_ms: 70.66700
19:12:10   severity: fail
19:12:10   exitcode: 1
19:12:10   stack: |-
19:12:10     node:internal/assert/utils:146
19:12:10       throw error;
19:12:10       ^
19:12:10     
19:12:10     AssertionError [ERR_ASSERTION]: Missing expected exception (TypeError).
19:12:10         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-whatwg-encoding-custom-textdecoder.js:88:10)
19:12:10         at Module._compile (node:internal/modules/cjs/loader:1803:14)
19:12:10         at Object..js (node:internal/modules/cjs/loader:1934:10)
19:12:10         at Module.load (node:internal/modules/cjs/loader:1524:32)
19:12:10         at Module._load (node:internal/modules/cjs/loader:1326:12)
19:12:10         at TracingChannel.traceSync (node:diagnostics_channel:328:14)
19:12:10         at wrapModuleLoad (node:internal/modules/cjs/loader:245:24)
19:12:10         at Module.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:154:5)
19:12:10         at node:internal/main/run_main_module:33:47 {
19:12:10       generatedMessage: false,
19:12:10       code: 'ERR_ASSERTION',
19:12:10       actual: undefined,
19:12:10       expected: {
19:12:10         code: 'ERR_NO_ICU',
19:12:10         name: 'TypeError',
19:12:10         message: '"fatal" option is not supported on Node.js compiled without ICU'
19:12:10       },
19:12:10       operator: 'throws',
19:12:10       diff: 'simple'
19:12:10     }
19:12:10     
19:12:10     Node.js v26.0.0-pre
19:12:10   ...

@ChALkeR
Copy link
Member Author

ChALkeR commented Jan 26, 2026

@richardlau indeed, I missed that, thanks! I'll update the test

@ChALkeR ChALkeR force-pushed the chalker/decoder/unify/1 branch from 701604e to 54e7ba2 Compare January 26, 2026 08:19
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@gurgunday gurgunday added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jan 26, 2026
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 27, 2026
@nodejs-github-bot nodejs-github-bot merged commit e155415 into nodejs:main Jan 27, 2026
70 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in e155415

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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

6 participants