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

@std/cli: move informational output to stderr (instead of stdout) #5495

Open
jsejcksn opened this issue Jul 19, 2024 · 8 comments
Open

@std/cli: move informational output to stderr (instead of stdout) #5495

jsejcksn opened this issue Jul 19, 2024 · 8 comments
Labels
cli suggestion a suggestion yet to be agreed

Comments

@jsejcksn
Copy link
Contributor

Refer to the comments beginning here for context: #5002 (comment)

It appears that some modules (e.g. spinner.ts and prompt_secret.ts, but there may be more) are still writing informational output to stdout. Is this really the intended behavior?

@iuioiua
Copy link
Contributor

iuioiua commented Jul 20, 2024

This sounds reasonable to me. I've created #5497 to play with the idea with Spinner. The errors need fixing.

This might be a little trickier to do with promptSecret(), as it's now a stable API (as of yesterday 😅).

WDYT, @kt3k?

@iuioiua iuioiua added suggestion a suggestion yet to be agreed cli and removed bug Something isn't working needs triage labels Jul 20, 2024
@kt3k
Copy link
Member

kt3k commented Jul 22, 2024

How about making the output destination optional (like, output: "stderr") instead of changing the default behavior?

@jsejcksn
Copy link
Contributor Author

How about making the output destination optional (like, output: "stderr") instead of changing the default behavior?

@kt3k In the linked issue that took place prior to stabilization, I explained why stdout should not be used for informational output. In short: writing to stdout by default will corrupt any expected structured output from a process (e.g. streams, piped data, structured logs, etc.)

@kt3k
Copy link
Member

kt3k commented Jul 22, 2024

We can do that for Spinner. But we'd like to avoid changing it for promptSecret as it's already stabilized.

will corrupt any expected structured output from a process (e.g. streams, piped data, structured logs, etc.)

I think this generally doesn't apply to promptSecret because when promptSecret is used, then stdin/stdout should be the terminal

@jsejcksn
Copy link
Contributor Author

But we'd like to avoid changing it for promptSecret as it's already stabilized.

I think this is unfortunate because these concerns were raised prior to stabilization, but no maintainer engaged them.

@iuioiua
Copy link
Contributor

iuioiua commented Jul 22, 2024

Apologies, @jsejcksn. Yes, this slipped under the radar before stabilization. But that might not be a big deal. As Yoshiya mentioned, we can make changes to Spinner, as it's unstable. I agree that promptSecret() should probably be kept as-is, but not because it's stable. Rather, promptSecret() should align with runtime behavior of prompt(), which outputs to stdin. So until prompt() changes, I don't think we should change promptSecret().

@jsejcksn
Copy link
Contributor Author

jsejcksn commented Jul 22, 2024

Rather, promptSecret() should align with runtime behavior of prompt(), which outputs to stdin. So until prompt() changes, I don't think we should change promptSecret().

@iuioiua I think you mean that it outputs to stdout? It looks like Deno uses rustyline for its prompt op, which appears to use stdout for output. Is the design intention of promptSecret to mimic Deno's implementation of prompt as closely as possible — with the exception of concealing the input? If so, then I agree with your reasoning here.

@iuioiua
Copy link
Contributor

iuioiua commented Jul 22, 2024

Sorry, yes, I mean't stdout. And yes, it is intentional for promptSecret() to mimic Deno's implementation of prompt(), except for concealing the input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli suggestion a suggestion yet to be agreed
Projects
None yet
Development

No branches or pull requests

3 participants