Skip to content

Conversation

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Nov 29, 2025

This reverts commit 20bcaa0 (#55275)

Fixes: #60888
Fixes: #59515
Fixes: #56542
Refs: #55275 (comment)
Refs: #55275 (comment)

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. needs-ci PRs that need a full CI run. labels Nov 29, 2025
@ChALkeR ChALkeR marked this pull request as ready for review November 29, 2025 07:19
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2025
@mcollina
Copy link
Member

cc @mertcanaltin maybe there is an alternative fix.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2025
@nodejs-github-bot
Copy link
Collaborator

@mertcanaltin
Copy link
Member

Thanks for catching this. I'm sorry for the trouble this has caused. Thank you for the detailed explanation, @ChALkeR.

@mcollina Thanks for the ping. I'll look into an alternative fix - I think separate mappings for the two encodings would be a good solution.

@ChALkeR
Copy link
Member Author

ChALkeR commented Nov 29, 2025

@mertcanaltin Is there any encoding in TextDecoder API which can utilize the Latin1 codepath?
My assumption is no, hence the removal of that method (as nothing else uses it)

@mertcanaltin
Copy link
Member

@mertcanaltin Is there any encoding in TextDecoder API which can utilize the Latin1 codepath? My assumption is no, hence the removal of that method (as nothing else uses it)

Yes, I just checked, and as you said, there is no valid use case for the Latin1 method in the TextDecoder API, so it can be removed.

@codecov
Copy link

codecov bot commented Nov 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.56%. Comparing base (d1ab5ef) to head (a76afd8).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #60889   +/-   ##
=======================================
  Coverage   88.55%   88.56%           
=======================================
  Files         703      703           
  Lines      208291   208250   -41     
  Branches    40174    40156   -18     
=======================================
- Hits       184454   184429   -25     
+ Misses      15829    15826    -3     
+ Partials     8008     7995   -13     
Files with missing lines Coverage Δ
lib/internal/encoding.js 99.50% <100.00%> (-0.01%) ⬇️
src/encoding_binding.cc 84.13% <ø> (+4.92%) ⬆️
src/encoding_binding.h 100.00% <ø> (ø)

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

@mertcanaltin
Copy link
Member

I tried a fix #60893
@mcollina @ChALkeR

@ChALkeR
Copy link
Member Author

ChALkeR commented Nov 29, 2025

I think this should be separate PRs, because:

  1. DecodeLatin1 is unused and can be removed, which src: implement Windows-1252 encoding support and update related tests #60893 doesn't do but this PR does. Upd: fixed
  2. To make backporting easier

Also tests from #58890 would be helpful (upd: #60893 covers that now!)

No objection for going ahead and landing #60893 though, but then it has to be backported to all branches which regressed

@ChALkeR
Copy link
Member Author

ChALkeR commented Nov 29, 2025

Let's go with #60893, it looks cleaner after an update

Not closing this PR until that lands though in case something arises on review

@mertcanaltin
Copy link
Member

Let's go with #60893, it looks cleaner after an update

Not closing this PR until that lands though in case something arises on review

Okay, thank you very much for your support and details. It was great working with you. 🚀

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++. encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. needs-ci PRs that need a full CI run.

Projects

None yet

5 participants