Skip to content
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: stdout and stderr encoding on Windows #14559

Merged
merged 8 commits into from
May 11, 2022

Conversation

dsherret
Copy link
Member

Given:

const encoder = new TextEncoder();

// stdout
{
  console.log("你好,世界");
  const size = await Deno.stdout.write(encoder.encode("🦕\n"));
  if (size !== 5) {
    throw new Error();
  }
}

// stderr
{
  console.error("你好,世界");
  const size = await Deno.stderr.write(encoder.encode("🦕\n"));
  if (size !== 5) {
    throw new Error();
  }
}

Before:

õ¢áÕÑ¢´╝îõ©ûþòî
­ƒªò
õ¢áÕÑ¢´╝îõ©ûþòî
­ƒªò

After:

你好,世界
🦕
你好,世界
🦕

Closes #6001
Closes #14516
Closes #14545

@dsherret
Copy link
Member Author

dsherret commented May 10, 2022

I tried to add a test for this, but whatever I tried passed before and after (I think probably because our test utilitity collects data with a pipe and so it's not a console and doesn't get this special behaviour applied)

Edit: Nevermind. Added a pty test.

@dsherret dsherret marked this pull request as ready for review May 10, 2022 18:18
@dsherret dsherret requested a review from bartlomieju as a code owner May 10, 2022 18:18
@dsherret dsherret requested review from piscisaureus and ry May 10, 2022 18:18
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - lack of test is unfortunate but I don't have any suggestions...

/// that is then cloned when obtaining stdio for process. In turn when
/// resource table is dropped storing reference to that handle, the handle
/// itself won't be closed (so Deno.core.print) will still work.
// TODO(ry) It should be possible to close stdout.
Copy link
Member

Choose a reason for hiding this comment

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

regarding the TODO is it possible to close stdout after this patch?

Copy link
Member Author

@dsherret dsherret May 11, 2022

Choose a reason for hiding this comment

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

No, not possible still. I removed the todo because I didn't think it was worth keeping it in the code since I think doing this will arise from the need to do it.

@dsherret
Copy link
Member Author

Actually, Bartek had the good idea of testing this in a pty and I just thought to repurpose the repl tests for this. I just tried it out and it fails on main, but passes on this branch. The downside of course is that it doesn't run on the CI, but better than nothing.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +211 to +220
// windows was having issues displaying this
util::with_pty(&["repl"], |mut console| {
console.write_line("console.log('🦕');");
console.write_line("close();");

let output = console.read_all_output();
// one for input, one for output
let emoji_count = output.chars().filter(|c| *c == '🦕').count();
assert_eq!(emoji_count, 2);
});
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants