Uh oh!
There was an error while loading. Please reload this page.
fs: accept all valid utf8 values in fast paths - #64341
Conversation
Signed-off-by: Hamid Reza Ghavami <hamidr.ghavami@gmail.com>
bfaade2 to
1d3f916CompareCodecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #64341 +/- ##
==========================================
- Coverage 92.01% 90.24% -1.77%
==========================================
Files 379 741 +362 Lines 166972 241706 +74734 Branches 25554 45562 +20008 ==========================================
+ Hits 153639 218128 +64489 - Misses 13041 15107 +2066 - Partials 292 8471 +8179
🚀 New features to boost your workflow:
|
anonrig
commented
Jul 7, 2026
I don't think we should land this. The more checks we have, the more performance impact it would create. Do you have any examples out there that is impacted by this? Does people really pass UTF8? If not, let's close this. |
hamidrezaghavami
commented
Jul 7, 2026
Hi @anonrig thanks for the review! I completely understand the concern regarding the performance impact on the fast path. I picked this up to resolve issue #49888, as there seemed to be some inconsistency in how variations like That said, if the consensus is that the branch/check overhead for the 99% of users passing standard |
Signed-off-by: Hamid Reza Ghavami <hamidr.ghavami@gmail.com>
1d3f916 to
fccaa91Comparehamidrezaghavami
commented
Jul 14, 2026
The commit message linting and sign-off trailer are fixed, and all CI checks are passing! Could someone from the @nodejs/fs team please take a look when you have a moment? Thanks! |
Fixes: #49888
This PR introduces a lightweight
isUtf8Encodinghelper to ensurefs.readFileSync,fs.writeFileSync, andencodeRealpathResultuse the fast C++ path for all valid variations of the utf8 encoding string (utf8,utf-8,UTF8,UTF-8).