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

[Bug]: Logging should be done to STDERR by default. STDOUT should be reserved for scriptable output. #3863

Open
2 tasks done
charlespwd opened this issue May 7, 2024 · 5 comments · May be fixed by #4922
Open
2 tasks done
Labels
Area: Housekeeping Area: @shopify/cli-kit @shopify/cli-kit package issues Type: Bug Something isn't working

Comments

@charlespwd
Copy link
Contributor

charlespwd commented May 7, 2024

Please confirm that you have:

  • Searched existing issues to see if your issue is a duplicate. (If you’ve found a duplicate issue, feel free to add additional information in a comment on it.)
  • Reproduced the issue in the latest CLI version.

In which of these areas are you experiencing a problem?

Other

Expected behavior

I noticed that logging is done by default to STDOUT (even debug logging, for theme check anyway). Shouldn’t that be sent to STDERR by default?

Logging to STDOUT when it isn’t meant to be captured by a script makes the CLI non-composable/non-scriptable.

For instance, shopify theme check --verbose --output json will have non-JSON output in STDOUT and makes | jq break.

IMO STDOUT should be intentional, not the default.

Actual behavior

IMO all of these should use STDERR by default. STDOUT is the wrong default.

  • outputDebug should log to STDERR by default (console.error)
  • outputInfo
  • outputWarn

Verbose output

$ shopify theme check --output json --verbose | jq .
jq: parse error: Invalid numeric literal at line 1, column 14

Reproduction steps

  1. shopify theme check --output json --verbose | jq . -- get malformed JSON because debug logging isn't JSON.

Operating System

All

Shopify CLI version (check your project's package.json if you're not sure)

3.60

@knjshimi
Copy link

knjshimi commented May 7, 2024

+1

Copy link
Contributor

This issue seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action.
→ If there's no activity within a week, then a bot will automatically close this.
Thanks for helping to improve Shopify's dev tooling and experience.

P.S. You can learn more about why we stale issues here.

@charlespwd
Copy link
Contributor Author

If we did this, we wouldn't have had Shopify/lighthouse-ci-action#87

@charlespwd charlespwd added Area: @shopify/cli-kit @shopify/cli-kit package issues and removed no-issue-activity labels Nov 25, 2024
@gonzaloriestra gonzaloriestra linked a pull request Nov 26, 2024 that will close this issue
5 tasks
@gonzaloriestra
Copy link
Contributor

I've created a draft PR with a potential solution for this: #4922, in case someone wants to continue

@charlespwd
Copy link
Contributor Author

I would even suggest having a new output helper named output that would be the only one that logs to STDOUT.

That way you can pick from the list:

  • outputInfo - STDERR loglevel info
  • outputDebug - STDERR loglevel debug
  • outputWarn - STDERR loglevel warn
  • outputError - STDERR loglevel error
  • output - STDOUT. Use only for JSON or whatever could be piped into another bash script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Housekeeping Area: @shopify/cli-kit @shopify/cli-kit package issues Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants