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

fix(ls/search) --json arg error also sent to stdout #3437

Closed

Conversation

RammasEchor
Copy link

@RammasEchor RammasEchor commented Jun 17, 2021

What

Added output to STDOUT via npm.output() when:

  • npm search --json: outputs an error to STDOUT in json format.
  • npm ls --json & a dependency can't be resolved: the json output to STDOUT has an error embedded.

Why

Preserve backwards compatibility for scripts done for 6.x that rely on an error in STDOUT.

References

Fixes #2740

@RammasEchor RammasEchor requested a review from a team as a code owner June 17, 2021 19:41
@RammasEchor RammasEchor changed the title fix(ls/search) --json arg error also sent to stdout #2740 fix(ls/search) --json arg error also sent to stdout Jun 17, 2021
@ljharb
Copy link
Contributor

ljharb commented Jun 17, 2021

Why would that be something desirable? Errors should never be printed on stdout, and scripts expecting it need correcting.

@darcyclarke darcyclarke added Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes labels Jun 21, 2021
@wraithgar
Copy link
Member

We don't have this pattern set up in any of our commands, to my knowledge. If we wanted --json mode to start sending errors to stdout, and in json, that would require some discussion at the very least through our rfc process

Leaving this open for now as a reference for a future discussion.

@wraithgar wraithgar added Needs Discussion is pending a discussion semver:major backwards-incompatible breaking changes and removed semver:patch semver patch level for changes labels Jun 23, 2021
@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Jun 23, 2021
@ljharb
Copy link
Contributor

ljharb commented Jun 23, 2021

@wraithgar if errors appear on stdout with --json that's always a bug - anyone setting --json is likely piping it to a json parser, which won't understand anything that's not in JSON itself.

@nlf
Copy link
Contributor

nlf commented Jun 23, 2021

i think i agree with @ljharb on this one, the behavior in npm 6 was that we hang errors off of the json object and print it as part of the json. npm 7 logging errors in non-json to any stream while printing json feels like a bug (and a regression).

@wraithgar
Copy link
Member

As discussed at today's open rfc meeting If we were to implement something like this, it would need to be designed from the ground up as a well-defined way to handle errors in --json mode, and then to make the cli conform to that spec.

If you have a solid idea for how npm should be handling errors (both expected and unexpected) please describe it in an rfc.

@wraithgar wraithgar closed this Jun 23, 2021
@RammasEchor
Copy link
Author

Thank you everyone for taking the time to review it, and thanks for letting me know why it was not accepted. Maybe next time!

@ruyadorno ruyadorno removed the Agenda will be discussed at the Open RFC call label Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Discussion is pending a discussion Release 7.x work is associated with a specific npm 7 release semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants