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.

Sign up for GitHub

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

Print logs to stderr #4922

Closed
wants to merge 2 commits into from
Closed

Print logs to stderr #4922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de4f34f
Print log messages to stderr
gonzaloriestra Nov 26, 2024
bd3d042
Add changeset
gonzaloriestra Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/silent-years-smash.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@shopify/ui-extensions-dev-console-app': patch
'@shopify/cli-kit': patch
'@shopify/theme': patch
'@shopify/app': patch
---

Print all log messages to stderr instead of stdout
6 changes: 3 additions & 3 deletions packages/app/src/cli/services/logs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {getAppConfigurationFileName} from '../models/app/loader.js'
import {DeveloperPlatformClient} from '../utilities/developer-platform-client.js'
import {Organization, OrganizationApp, OrganizationStore} from '../models/organization.js'
import {AbortError} from '@shopify/cli-kit/node/error'
import {consoleLog} from '@shopify/cli-kit/node/output'
import {consoleLog, consoleWarn} from '@shopify/cli-kit/node/output'
import {renderInfo} from '@shopify/cli-kit/node/ui'
import {basename} from '@shopify/cli-kit/node/path'

Expand Down Expand Up @@ -67,7 +67,7 @@ export async function logs(commandOptions: LogsOptions) {

if (commandOptions.format === 'json') {
consoleLog(JSON.stringify({subscribedToStores: commandOptions.storeFqdns}))
consoleLog(JSON.stringify({message: 'Waiting for app logs...'}))
consoleWarn(JSON.stringify({message: 'Waiting for app logs...'}))
await renderJsonLogs({
options: {
variables,
Expand All @@ -77,7 +77,7 @@ export async function logs(commandOptions: LogsOptions) {
storeNameById: logsConfig.storeNameById,
})
} else {
consoleLog('Waiting for app logs...\n')
consoleWarn('Waiting for app logs...\n')
await renderLogs({
options: {
variables,
Expand Down
14 changes: 6 additions & 8 deletions packages/cli-kit/src/public/node/output.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,7 @@ export function collectLog(key: string, content: OutputMessage): void {
}

export const clearCollectedLogs = (): void => {
// console.log('clearCollectLogs')
collectedLogs = {}
// console.log(collectedLogs)
}

/**
Expand All @@ -258,7 +256,7 @@ export const clearCollectedLogs = (): void => {
* @param content - The content to be output to the user.
* @param logger - The logging function to use to output to the user.
*/
export function outputInfo(content: OutputMessage, logger: Logger = consoleLog): void {
export function outputInfo(content: OutputMessage, logger: Logger = consoleWarn): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

May I suggest a export function output(content: outputMessage, logger: Logger = consoleLog): void {

?

That way all the output{log,warn,error} would all go to stderr and we'd make it explicit which one goes to stdout?

STDOUT = scriptable output
STDERR = logging

Copy link
Contributor

Choose a reason for hiding this comment

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

console.log goes to stdout...

I think the idea is that we have several output functions based on log level, but by default all log to consoleWarn which uses console.warn which goes to stderr.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's what I'm saying. Let's change this so that console.log is only used intentionally.

  • 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should ever log anything without specifying a log level. We can default everything to stderr and make stdout an explicit choice by specifying stdout as the logger.

Copy link
Contributor

Choose a reason for hiding this comment

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

idk I think output is output and logging is logging. I personally don't see a problem with these two things:

  • logging that goes to STDERR and requires a loglevel
  • output goes to STDOUT and doesn't require a loglevel.

STDOUT is for piping & scripts. Why would we want a loglevel in there? What am I missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because, while we don't currently support it, many programs have a silent log level which doesn't print anything. And I think we should ultimately support that. Keeping everything with log levels gives us more room to fine-tune logging behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a good example: https://docs.npmjs.com/cli/v10/using-npm/config#loglevel

Silent is just another log level.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't that mean that --json --silent do nothing at all? There's always &> /dev/null?

Whenever I'm scripting, I'm mostly annoyed by shit showing up in STDOUT and that's why I'd want --silent, but if it doesn't, then I don't see the point.

One of pnpm or nx is an example offender here. If I want to run the shopify theme language-server with the dev build in Shopify/cli, I can only do that by running the build node package stuff because otherwise pnpm & nx are outputting crap to STDOUT that don't fit the LSP.

Same goes for pnpm shopify theme check --json.

Copy link
Contributor

@charlespwd charlespwd Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For that reason, whenever I want to test a Shopify/theme-tools bump in vim. I have to use a script in my $HOME/bin that is this:

#!/usr/bin/env bash

if [[ -z $DEBUG ]]; then
  node /Users/charles/src/github.com/Shopify/cli/packages/cli/bin/dev.js "$@"
else
  node --inspect-brk /Users/charles/src/github.com/Shopify/cli/packages/cli/bin/dev.js "$@"
fi

Which means I need pnpm run build any time I want to test it too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried pnpm --silent shopify:run ...

--silent quiets pnpm

shopify:run skips the build step (you have to run pnpm shopify build first, but anyway you need to do that in your script)

const message = stringifyMessage(content)
if (isUnitTest()) collectLog('info', content)
outputWhereAppropriate('info', logger, message)
Expand Down Expand Up @@ -300,7 +298,7 @@ export function outputCompleted(content: OutputMessage, logger: Logger = console
* @param content - The content to be output to the user.
* @param logger - The logging function to use to output to the user.
*/
export function outputDebug(content: OutputMessage, logger: Logger = consoleLog): void {
export function outputDebug(content: OutputMessage, logger: Logger = consoleWarn): void {
if (isUnitTest()) collectLog('debug', content)
const message = colors.gray(stringifyMessage(content))
outputWhereAppropriate('debug', logger, `${new Date().toISOString()}: ${message}`)
Expand All @@ -324,7 +322,7 @@ export function outputWarn(content: OutputMessage, logger: Logger = consoleWarn)
* Prints a new line in the terminal.
*/
export function outputNewline(): void {
console.log()
console.warn()
}

/**
Expand Down Expand Up @@ -366,7 +364,7 @@ export interface OutputProcess {
}

/**
* Prints a log message in the console.
* Prints a log message in the console to stdout.
*
* @param message - The message to print.
*/
Expand All @@ -375,7 +373,7 @@ export function consoleLog(message: string): void {
}

/**
* Prints an error message in the console.
* Prints an error message in the console to stderr.
*
* @param message - The message to print.
*/
Expand All @@ -384,7 +382,7 @@ export function consoleError(message: string): void {
}

/**
* Prints a warning message in the console.
* Prints a warning message in the console to stderr.
*
* @param message - The message to print.
*/
Expand Down
4 changes: 2 additions & 2 deletions packages/theme/src/cli/services/console.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {ensureValidPassword} from '../utilities/theme-environment/storefront-pas
import {replLoop} from '../utilities/repl/repl.js'
import {initializeDevServerSession} from '../utilities/theme-environment/dev-server-session.js'
import {AdminSession} from '@shopify/cli-kit/node/session'
import {consoleLog} from '@shopify/cli-kit/node/output'
import {consoleWarn} from '@shopify/cli-kit/node/output'

export async function ensureReplEnv(adminSession: AdminSession, storePasswordFlag?: string) {
const themeId = await findOrCreateReplTheme(adminSession)
Expand Down Expand Up @@ -33,7 +33,7 @@ export async function initializeRepl(
themeAccessPassword?: string,
storefrontPassword?: string,
) {
consoleLog('Welcome to Shopify Liquid console\n(press Ctrl + C to exit)')
consoleWarn('Welcome to Shopify Liquid console\n(press Ctrl + C to exit)')

const session = await initializeDevServerSession(themeId, adminSession, themeAccessPassword, storefrontPassword)

Expand Down
2 changes: 1 addition & 1 deletion packages/ui-extensions-dev-console/src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const i18nManager = new I18nManager({
locale: 'en',
onError(error) {
// eslint-disable-next-line no-console
console.log(error)
console.error(error)
},
})

Expand Down