src: use wide string for findPackageJson onWindows #55861
Conversation
nodejs-github-bot
added
c++
|
Can you add a test? |
islandryu
force-pushed
the
fix/nonAscii
branch
from
341da94
to
19f43e3
Compare
Fix error when searching for package.json with non-ASCII characters in paths Fixes: nodejs#55773
islandryu
force-pushed
the
fix/nonAscii
branch
from
19f43e3
to
d0e4106
Compare
test/parallel/test-non-ascii.js
Outdated
| fs.mkdirSync(tmpdir.resolve('12月'), { recursive: true }); | ||
| fs.writeFileSync( | ||
| tmpdir.resolve('12月/index.js'), | ||
| "console.log('12月');", | ||
| ); |
There was a problem hiding this comment.
This can be a fixture, right?
There was a problem hiding this comment.
Thank you. I should do that.
Co-authored-by: Aviv Keller <redyetidev@gmail.com>
Co-authored-by: Aviv Keller <redyetidev@gmail.com>
Co-authored-by: Aviv Keller <redyetidev@gmail.com>
Co-authored-by: Aviv Keller <redyetidev@gmail.com>
| 'use strict'; | ||
|
|
||
| const fs = require('node:fs'); | ||
| const common = require('../common'); |
There was a problem hiding this comment.
Common must be the first import. FWIW running make lint will catch this.
There was a problem hiding this comment.
sorry for the lint error.
I've fixed it.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55861 +/- ##
==========================================
- Coverage 88.42% 87.93% -0.49%
==========================================
Files 654 654
Lines 187852 188038 +186
Branches 36134 35871 -263
==========================================
- Hits 166102 165350 -752
- Misses 14989 15868 +879
- Partials 6761 6820 +59
|
src/util.h
Outdated
| std::wstring ConvertToWideString(const std::string& str); | ||
|
|
||
| #define BufferValueToPath(str) \ | ||
| std::filesystem::path(ConvertToWideString(str.ToString())) |
There was a problem hiding this comment.
These macros are unnecessary. Also, str.ToString() makes an unnecessary copy, even tho, the function is const std::string&. str.ToStringView() would remove the unnecessary copy here.
There was a problem hiding this comment.
I modified the code to avoid using macros and to use std::string_view instead.
islandryu
force-pushed
the
fix/nonAscii
branch
from
ee8b68c
to
6dfd273
Compare
islandryu
force-pushed
the
fix/nonAscii
branch
from
6dfd273
to
c295818
Compare
| @@ -0,0 +1 @@ | |||
| console.log('check non-ascii'); | |||
There was a problem hiding this comment.
During testing, it became evident that the occurrence of the error described in issue #55773 is influenced by the character encoding used by the user.
For instance, users working with CP1252 are unlikely to encounter any errors, regardless of the characters they use. In contrast, users with encodings like CP932 are more prone to experiencing errors.
Take the character "月" as an example, which is represented in UTF-8 as:
E6 9C 88
In CP932, the byte 88 is interpreted as the leading byte of a 2-byte character, potentially causing issues.
The key takeaway is that to ensure accurate regression testing, it might be necessary to test the runtime environment with character encodings other than CP1252.
I also looked for other tests that might include regression testing against Windows-specific character encodings but couldn’t find any. If you have any good ideas, I’d appreciate your input.
Fix error when searching for package.json with non-ASCII characters in paths
Fixes: #55773