switch to use iconv-lite-umd for #79275#100472
switch to use iconv-lite-umd for #79275#100472bpasero merged 2 commits intomicrosoft:masterfrom gyzerok:use-iconv-lite-umd
Conversation
| const decoder = iconv.getDecoder(encoding); | ||
| process.stdin.on('error', stdinFileStream.destroy); | ||
| process.stdin.on('data', (chunk) => { | ||
| stdinFileStream.write(decoder.write(chunk)); | ||
| }); | ||
| process.stdin.on('end', () => { | ||
| stdinFileStream.write(decoder.end()); | ||
| stdinFileStream.end(); | ||
| }); | ||
| process.stdin.on('close', stdinFileStream.close); |
There was a problem hiding this comment.
@bpasero I am not sure how correct is what I am trying to achieve here. Also I am not entirely sure what would be the best way to test that everything is working as expected.
Alternatively we can change iconv-lite-umd to expose streaming interface and keep code here as it was.
Can you help me here to figure out which route to take?
There was a problem hiding this comment.
@gyzerok a way to test this is to run ps aux | grep code | ./scripts/code-cli.sh - which will pipe something into VSCode. The idea here is that if the terminal is using a non-utf8 encoding, we need to convert it.
I think your code is doing OK, I looked into that earlier. Any objections to that path?
There was a problem hiding this comment.
@bpasero yeah, seems to work fine then. I prefer this way better because if we don't expose streaming API from iconv-lite-umd there is not chance that someone will accidentally use it in the wrong environment.
Can you restart the failed job? It looks like it was just some random failure. Otherwise it's ready then.
There was a problem hiding this comment.
Actually nvm with the job. I'll squash commits for cleaner history.
|
Thanks! Nevermind the commits, I always "Squash and merge" PRs typically. |
2 participants
This is next PR in a series for #79275
Update: Didn't get any test failure locally. Will fix it.