-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(prompt): fix prompt output of non-ascii characters on windows #8199
Conversation
} else { | ||
print!("{}", str_.to_rust_string_lossy(tc_scope)); | ||
stdout().flush().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added flush here because print! macro doesn't seem flushing automatically. rust-lang/rust#23818
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kt3k this is a bit strange - Deno.core.print()
works for me without a problem without the flush()
and it's one of purposes of Deno.core.print
not to include new line by default. Could you explain the reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bartlomieju print!
is line-buffered as discussed in rust-lang/rust#23818. So if we use it like Deno.core.print('hello')
, the string doesn't show in stdout until new line is printed or process is finished. (Try the script Deno.core.print('xyz'); setTimeout(() => {}, 4000);
for example).
If we don't add flush()
here, prompt
works like the below:
> prompt('hi!')
hello
hi! "hello"
(The answer and question are reversed here.) I think this isn't what the users expect.
If we add flush()
, it works like:
> prompt('hi!')
hi! hello
"hello"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kt3k I can not reproduce that:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In repl, the returned value is printed with line break (such as "1\n") and this line break should trigger flushing of stdout.
Try the following:
deno eval "Deno.core.print('xyz'); setTimeout(() => {}, 4000);"
Validated on Windows
|
It'd be nice if stdout (when attached to a terminal) actually behaved as expected and interpreted the byte stream as UTF8-encoded text. But this fix is also legit, and the thing it fixes is really an eyesore. So LGTM from me. |
Deno.stdout.writeSync(new TextEncoder().encode(str))
doesn't seem working for non-ascii characters on windows, butDeno.core.print(str)
does seem working. See the comment #8179 (comment) .So this causes the issue #8179.
This PR replaced the use of
Deno.stdout.writeSync
in prompt withDeno.core.print
and avoided the issue.I checked the fix on window server 2019 machine.

fixes #8179