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

chore: warn to stdout instead of stderr #580

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

josgraha
Copy link

Motivation

Please always log actionable text to STDOUT instead of STDERR. As per The Open Group Base Specification on standard streams, stderr is used to indicate a non-zero exit code. Some package managers like rush consider stderr output to indicate a non-zero exit status and probably others do as well following a convention to return the highest FILENO that is used as the exit status.

Notable verbiage
TOG-Spec-STD-IO-Streams

I think the intention of the outdated caniuse-lite package warning is to provide actionable next steps to the user and not create an implied exit status when displayed.

Supporting evidence

Searching Stack Overflow for the text Browserslist: caniuse-lite is outdated reveals many users confused why their builds are breaking and probably trying to do the right thing which is update their dependencies. However I think it's worthwhile to consider the implications of which stream is being used and what exit status that implies.

Reproducible steps

  • create a rush project that has an outdated caniuse-lite browser list
  • add an npm build script rush will invoke using built-in command and will generates the warning (similar to lerna run build but build is sugar the script name. (note: the built-in command will use the default settings)
  • add some some other script (example: dist)
  • string together the rush build target and npm script and the with && (example)
rush build && pnpm run dist

Expected

  • dist was run

Actual

  • dist will not run as a non-zero exit status short circuits the logical expression

Changelog

  • replace console.warn with console.info in regards
  • update tests to spy on info

Thanks so much for your time in considering this PR and I hope I speak for other users and not just my little corner of the world with the Rush Stack.

@ai
Copy link
Member

ai commented Jan 28, 2021

Should we change the console.warn() behavior on the Node.js level in this case?

Semantically, it is warning, not an info. It is not a big deal for me where to print it. But why Node.js prints warnings to stderr?

@ai
Copy link
Member

ai commented Jan 28, 2021

What if we will check typeof process and print warning by process.stdout.write and use console.warn() in other cases?

@josgraha
Copy link
Author

josgraha commented Feb 1, 2021

Should we change the console.warn() behavior on the Node.js level in this case?

Semantically, it is warning, not an info. It is not a big deal for me where to print it. But why Node.js prints warnings to stderr?

Well you nailed it right there, it basically comes down to semantics, is this an informative call to action or diagnostic information that should be interleaved with diagnostic information of other packages used by downstream consumers? As an application developer, I would prefer having this diagnostic information explicitly displayed, similar to what we would expect when we lint codebase. That is I want to "opt-in" to the diagnostic logging. In such a case I am asking the package for explicit diagnostic information that I can leverage with tooling to determine the state of the build (or whatever explicit script is being invoked). To reiterate the utility, we can opt-in or opt-out of diagnostic information. This isn't an option here, we are opted-in via post-install or whatever lifecycle hook invokes this code path.

@josgraha
Copy link
Author

josgraha commented Feb 1, 2021

What if we will check typeof process and print warning by process.stdout.write and use console.warn() in other cases?

Ah right in the case of UMD or bundled scripts in the browser? Didn't even think about that, great suggestion.

@ai
Copy link
Member

ai commented Feb 1, 2021

Ah right in the case of UMD or bundled scripts in the browser?

Yeap, there are use cases, when Browserslist called in the browser

@romainmenke
Copy link

Please don't do this :)
This was conceptually resolved in the 70ties by Ken Thompson.

stdout is for program output not for diagnostics.

If there is a program that interprets stderr outputs as a non-zero exit code it should be resolved in that program.

https://www.cs.dartmouth.edu/~doug/reader.pdf

Screenshot 2022-02-11 at 12 34 35

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

Successfully merging this pull request may close these issues.

None yet

3 participants