FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: unify ICU and no-ICU TextDecoder by ChALkeR · Pull Request #61409 · nodejs/node · GitHub

/ node Public

lib: unify ICU and no-ICU TextDecoder - #61409

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
ChALkeR:chalker/decoder/unify/1
Jan 27, 2026
ConversationCommits (2)ChecksFiles changed
Merged

lib: unify ICU and no-ICU TextDecoder#61409
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
ChALkeR:chalker/decoder/unify/1

Conversation

ChALkeR commented Jan 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Jan 17, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup

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 changed the title Chalker/decoder/unify/1 lib: unify ICU and no-ICU TextDecoder Jan 17, 2026
ChALkeR force-pushed the chalker/decoder/unify/1 branch 2 times, most recently from 367908c to 5a2317f Compare January 17, 2026 14:24
ChALkeR commented Jan 17, 2026
View reviewed changes
Comment thread lib/internal/encoding.js Outdated
ChALkeR mentioned this pull request Jan 17, 2026
Closed
ChALkeR force-pushed the chalker/decoder/unify/1 branch 5 times, most recently from 4d6ff8f to b2e1ece Compare January 17, 2026 23:54
ChALkeR marked this pull request as ready for review January 17, 2026 23:55
ChALkeR force-pushed the chalker/decoder/unify/1 branch 7 times, most recently from adb2410 to fb0ca32 Compare January 18, 2026 00:29

codecov Bot commented Jan 18, 2026 •
edited
Loading

Copy link
Copy Markdown

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.

avivkeller reviewed Jan 19, 2026
View reviewed changes
Comment thread lib/internal/encoding.js
Comment on lines +420 to +425
let StringDecoder;
function lazyStringDecoder() {
if (StringDecoder === undefined)
({ StringDecoder } = require('string_decoder'));
return StringDecoder;
}

avivkeller Jan 19, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There's a lazy utility in the internal utils.

ChALkeR Jan 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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

anonrig Jan 23, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ChALkeR Jan 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

ChALkeR Jan 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

#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 requested a review from avivkeller January 20, 2026 00:04
ChALkeR force-pushed the chalker/decoder/unify/1 branch from fb0ca32 to 1204484 Compare January 21, 2026 13:55
ChALkeR force-pushed the chalker/decoder/unify/1 branch from 1204484 to e74c6fe Compare January 21, 2026 19:25

ChALkeR commented Jan 21, 2026

Copy link
Copy Markdown
Member Author

Rebased, no actual changes

gurgunday approved these changes Jan 25, 2026
View reviewed changes

gurgunday 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

gurgunday added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 25, 2026
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 commented Jan 25, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/71011/

nodejs-github-bot commented Jan 25, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/71031/

richardlau commented Jan 25, 2026

Copy link
Copy Markdown
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 commented Jan 26, 2026

Copy link
Copy Markdown
Member Author

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

ChALkeR force-pushed the chalker/decoder/unify/1 branch from 701604e to 54e7ba2 Compare January 26, 2026 08:19

nodejs-github-bot commented Jan 26, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/71041/

nodejs-github-bot commented Jan 26, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/71050/

gurgunday added author ready PRs that have at least one approval, no outstanding review comments, 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 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 27, 2026
nodejs-github-bot merged commit e155415 into nodejs:main Jan 27, 2026
70 checks passed

nodejs-github-bot commented Jan 27, 2026

Copy link
Copy Markdown
Collaborator

Landed in e155415

gurgunday mentioned this pull request Jan 27, 2026
Open
This was referenced Jan 27, 2026
Closed
Open
aduh95 pushed a commit that referenced this pull request Jan 28, 2026
lib: unify ICU and no-ICU TextDecoder
a782598 
PR-URL: #61409
Reviewed-By: Aviv Keller <me@aviv.sh>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Richard Lau <richard.lau@ibm.com>
github-actions Bot mentioned this pull request Feb 2, 2026
Merged
aduh95 pushed a commit that referenced this pull request Feb 10, 2026
lib: unify ICU and no-ICU TextDecoder
bab6374 
PR-URL: #61409
Reviewed-By: Aviv Keller <me@aviv.sh>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Richard Lau <richard.lau@ibm.com>
github-actions Bot mentioned this pull request Feb 22, 2026
Merged
aduh95 pushed a commit that referenced this pull request Feb 22, 2026
lib: unify ICU and no-ICU TextDecoder
7824c75 
PR-URL: #61409
Reviewed-By: Aviv Keller <me@aviv.sh>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Richard Lau <richard.lau@ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

anonrig anonrig approved these changes
richardlau richardlau approved these changes
avivkeller avivkeller approved these changes
gurgunday gurgunday approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

6 participants

Back | FazBrowse Home | New Git URL