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

[BREAKING] Replace console.log with process.stderr #686

Merged
merged 13 commits into from
Feb 29, 2024
Merged

Conversation

d3xter666
Copy link
Contributor

@d3xter666 d3xter666 commented Feb 22, 2024

BREAKING CHANGE:
System messages will now be written to stderr instead of stdout.

JIRA: CPOUI5FOUNDATION-802
Related to: SAP/ui5-tooling#701
Sibling of: SAP/ui5-server#643, SAP/ui5-tooling#930

@coveralls
Copy link

coveralls commented Feb 22, 2024

Coverage Status

coverage: 96.571% (+0.1%) from 96.432%
when pulling d707261 on breaking-v4
into 6a8c7a6 on main.

@codeworrior codeworrior changed the title [BRAKING] Replace console.log with process.stderr [BREAKING] Replace console.log with process.stderr Feb 22, 2024
@flovogt
Copy link
Member

flovogt commented Feb 23, 2024

@RandomByte what was the reason here when implementing the logging here not choosing ui5-logger?

@d3xter666
Copy link
Contributor Author

@RandomByte what was the reason here when implementing the logging here not choosing ui5-logger?

My assumption is that we want "clear" messages and always to output something. This is the code in the logger:
https://github.com/SAP/ui5-logger/blob/d060f478b811fbf67f7452f60c9565f9fe01dab3/lib/loggers/Logger.js#L152

Basically, if the log level is silent, for example, nothing would occur and there will always be [info], [warning], etc. prepended in front of the message.

If my assumptions are invalid, please correct me and if there are no other objections, I'm perfectly fine to go with the @ui5/logger module

@RandomByte
Copy link
Member

@RandomByte what was the reason here when implementing the logging here not choosing ui5-logger?

My assumption is that we want "clear" messages and always to output something. This is the code in the logger: https://github.com/SAP/ui5-logger/blob/d060f478b811fbf67f7452f60c9565f9fe01dab3/lib/loggers/Logger.js#L152

Basically, if the log level is silent, for example, nothing would occur and there will always be [info], [warning], etc. prepended in front of the message.

If my assumptions are invalid, please correct me and if there are no other objections, I'm perfectly fine to go with the @ui5/logger module

I'm pretty sure this is a relic from the time where @ui5/logger was based on npmlog. Messages where buffered so we feared that in case of an exception, our error log message would not be printed. Also the chalk formatting might have caused trouble since npmlog did its own formatting.

The fact that we used console.log (writing to stdout instead of stderr) was a mistake I think.

Yavor perfectly outlined the differences. But I somehow suspect that a user would expect that for log level "silent" we don't even log error messages.

If we don't see any technical issues with switching to @ui5/logger here, I would prefer that. Also see the internal PR I just referenced you in.

@matz3
Copy link
Member

matz3 commented Feb 27, 2024

When I run ui5, I get this output (after fixing the broken import in init.js:

ui5
Command Failed:
Command Failed:
Command Failed:
Command Failed:
Command Failed:
Command Failed:
Command Failed:
Command Failed:
Command Failed:
Command required
Command required
Command required
Command required
Command required
Command required
Command required
Command required
Command required









See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'

Am I doing something wrong?

bin/ui5.cjs Show resolved Hide resolved
bin/ui5.cjs Show resolved Hide resolved
lib/cli/commands/init.js Outdated Show resolved Hide resolved
@RandomByte
Copy link
Member

Yavor and myself just discussed this and came to the conclusion to revert back to directly writing the error messages to process.stderr.

This is already an improvement from before, where errors were written to stdout.

The main rational for not using @ui5/logger is the added [error] prefix in every line which reduces readability. The CLI is not intended to be consumed as a node module, so we can safely use process.stdout and stderr directly.

Some of the commands always write their output do stdout (ui5 tree) for example. So it's reasonable to have catched exceptions being written to stderr with the CLI's own formatting logic.

@d3xter666
Copy link
Contributor Author

When I run ui5, I get this output (after fixing the broken import in init.js:

ui5
Command Failed:
Command Failed:
Command Failed:
Command Failed:
Command Failed:
Command Failed:
Command Failed:
Command Failed:
Command Failed:
Command required
Command required
Command required
Command required
Command required
Command required
Command required
Command required
Command required









See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'
See 'ui5 --help'

Am I doing something wrong?

It was just a try to provide cleaner messages for the Logger. It seems that if we want to achieve this via the Logger, the complexity of the code would increase. We discussed this with @RandomByte and it seems unreasonable to continue with that. Reverting back to process.stderr implementation instead.

Copy link
Member

@RandomByte RandomByte left a comment

Choose a reason for hiding this comment

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

Just went over the PR again. I think whenever we write the "main result" of a command, we should write it to stdout.

Any additional log messages as well as error messages should go to stderr. Basically anything that can't be parsed the same way as the standard output of the command.

lib/cli/commands/tree.js Outdated Show resolved Hide resolved
lib/cli/commands/tree.js Show resolved Hide resolved
lib/cli/commands/versions.js Outdated Show resolved Hide resolved
lib/cli/commands/use.js Outdated Show resolved Hide resolved
lib/cli/commands/init.js Outdated Show resolved Hide resolved
lib/cli/commands/remove.js Outdated Show resolved Hide resolved
lib/cli/commands/add.js Outdated Show resolved Hide resolved
lib/cli/commands/serve.js Outdated Show resolved Hide resolved
Copy link
Member

@RandomByte RandomByte left a comment

Choose a reason for hiding this comment

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

Looks good to me and matches my expectations. Thanks!

d3xter666 added a commit to SAP/ui5-server that referenced this pull request Feb 29, 2024
BREAKING CHANGE:
Messages will now be written to stderr instead of stdout.

JIRA: CPOUI5FOUNDATION-802
Related to: SAP/ui5-tooling#701
Sibling of: SAP/ui5-tooling#930,
SAP/ui5-cli#686
@d3xter666 d3xter666 merged commit 48d1975 into main Feb 29, 2024
17 checks passed
@d3xter666 d3xter666 deleted the breaking-v4 branch February 29, 2024 07:11
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.

5 participants