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

[Feature] Flag to suppress ➤ YN0000: output in yarn workspaces foreach #2517

Closed
2 tasks done
sargunv opened this issue Feb 25, 2021 · 16 comments · Fixed by #5152
Closed
2 tasks done

[Feature] Flag to suppress ➤ YN0000: output in yarn workspaces foreach #2517

sargunv opened this issue Feb 25, 2021 · 16 comments · Fixed by #5152
Labels
enhancement New feature or request

Comments

@sargunv
Copy link
Contributor

sargunv commented Feb 25, 2021

  • I'd be willing to implement this feature (contributing guide)
  • This feature is important to have in this repository; a contrib plugin wouldn't do

Describe the user story

Context: I have a Typescript project with many interdependent workspaces, most of which are libraries for a handful of applications. In CI, I use the following command to build any particular app with all its workspace dependencies:

yarn workspaces foreach -vpt --topological-dev -j 100 run build

Each workspace's build script calls tsc with some flags.

I use GitHub Actions as my CI with the https://github.com/actions/setup-node action to configure my CI environment for building my project. That setup step by default includes a tsc problem matcher which uses regex to match the output of the Typescript compiler and annotate the code in a PR with the actual errors.

If my CI runs yarn run build on a workspace directly, the output is identical to the output of tsc, so the matcher works fine. However, since I use yarn workspaces foreach, the output gets prefixed with Yarn's log level codes, causing the problem matcher to not match the output correctly. In particular, it breaks on matching the file name, causing the annotation to end up in the wrong place.

image

Describe the solution you'd like

I think yarn workspaces foreach should have a flag to suppress stuff like ➤ YN0000: from the output, or in other words a flag to get the raw unmodified output of the command(s) being run:

yarn workspaces foreach -vpt --topological-dev -j 100 --raw-output run build

I'd be willing to work on this PR if it's something that would be accepted.

Describe the drawbacks of your solution

Just the general drawback of having yet another flag/feature to maintain/test.

I don't think there are any specific drawbacks to this feature in particular, but I'm unfamiliar with Yarn's codebase so I likely have blind spots here.

Describe alternatives you've considered

This could be solved by modifying the regex in the problem matcher, but I believe it's out of scope for a tsc problem matcher to account for all kinds of wrappers that might be mutating the compiler's error format.

This could also be worked around by piping the output through another command that removes that prefix from the output, but my intuition is that'd be prone to failure. I imagine parsing the output of CLI tools in some way is a common enough use case that Yarn should have the flag to allow it without strange workarounds.

@arcanis
Copy link
Member

arcanis commented Feb 25, 2021

I don't remember why the prefix is printed (perhaps @deini remembers?), so it's at least a sign it should be investigated. From a quick look it seems to be because we sometimes can reach states where errors are thrown. Still, it's a fringe case and I could be swayed into replacing StreamReport by a simple ThrowReport (which would forward the output unmodified, and would simply throw any error).

@deini
Copy link
Member

deini commented Feb 25, 2021

If I recall correctly, we were trying to be consistent and use StreamReport. I don't believe it was for anything in particular.

@sargunv
Copy link
Contributor Author

sargunv commented Feb 25, 2021

Imo it makes most sense to be consistent with the regular commands, so when the non-foreach version of a command would log raw output, the foreach should too, and when the non-foreach version of a command would log via StreamReport, the foreach should too.

I thought changing the logging format of foreach run now would be considered a breaking change, so I figured a flag makes most sense.

That doesn't quite answer the question of how to handle errors thrown specifically out of the foreach logic though, and I'm not sure how to handle that. As another wrapper, what does yarn dlx do?

@bfricka
Copy link

bfricka commented Mar 3, 2021

I wouldn't be against removing YN**** from all output. It's hard to imagine it being useful for the vast majority of standard commands and use cases.

@arcanis
Copy link
Member

arcanis commented Mar 3, 2021

It won't be removed from the rest of the commands (except perhaps on a case-by-case basis), but for foreach let's do this. Other commands will have enableMessageNames anyway.

Imo it makes most sense to be consistent with the regular commands, so when the non-foreach version of a command would log raw output, the foreach should too, and when the non-foreach version of a command would log via StreamReport, the foreach should too.

It already does, but the prefix is added twice: one for foreach, and one for the command itself. The foreach prefix really has no useful purpose I can see.

@cusxio
Copy link

cusxio commented Apr 4, 2021

Just to add, here's another value from having the option to suppress it.

The YN0000 is actually quite noisy when used with concurrently.

[TSC]
[TSC] 1:55:43 PM - Starting compilation in watch mode...
[TSC]
[TSC]
[TSC] 1:55:44 PM - Found 0 errors. Watching for file changes.
[WS] ➤ YN0000: [@app/server]: [nodemon] 2.0.7
[WS] ➤ YN0000: [@app/server]: [nodemon] to restart at any time, enter `rs`
[WS] ➤ YN0000: [@app/server]: [nodemon] watching path(s): dist/**/*.js
[WS] ➤ YN0000: [@app/server]: [nodemon] watching extensions: js,mjs,json
[WS] ➤ YN0000: [@app/server]: [nodemon] starting `node dist/index.js`
[WS] ➤ YN0000: [@app/client]: ready - started server on 0.0.0.0:3000, url: http://localhost:3000
[WS] ➤ YN0000: [@app/client]: info  - Using webpack 4. Reason: future.webpack5 option not enabled https://nextjs.org/docs/messages/webpack5
[WS] ➤ YN0000: [@app/server]: started server on http://localhost:3001
[WS] ➤ YN0000: [@app/client]: event - compiled successfully

@daniel-brenot
Copy link

daniel-brenot commented Aug 26, 2021

Alright friends. In the meantime i have a very non-optimal solution.

First, make sure you run export FORCE_COLOR=true in the environment where this is going to run if you want to preserve your terminal colours. Otherwise, chaulk will notice that you are not outputting straight to a terminal and wont print colour.

Next you need to pipe the output into the cut command. For example:

yarn workspaces foreach -pv run build | cut -c 31-

This will trim the first 31 columns, which is to say that is how many characters there are making up the string '➤ YN0000: ' when you have colour enabled. After that, your output should now be missing the annoying code.

Yarn, please remove this or add an option to suppress this. It looks really ugly in my companies CI pipeline and is a super hacky solution.

@arcanis
Copy link
Member

arcanis commented Aug 26, 2021

As mentioned in my last post, we accept PRs on this 😉

@daniel-brenot
Copy link

As mentioned in my last post, we accept PRs on this 😉

Would you consider making the messages showing dependent on the enableMessageNames an adequate change then? If so, I'll make a pr in the next while to address the issue.

@daniel-brenot
Copy link

Actually, after setting the appropriate enableMessageNames to false, the foreach no longer shows this. This can be closed now as far as i can tell.

@johnameyer
Copy link

Would love to see a flag for yarn workspaces list as well rather than having to transform the NDJSON stream

tony added a commit to tony/cv that referenced this issue Sep 5, 2022
Even though we may not ultimately use it:

- It seems to be a project in the midst of a philosophical problem.
- Why give it another name, unless losing track of where it came from
- It seems to decouple architecture (yay) but I'm concerned
  with not adopting an incremental approach to make yarn v1 better.

  Why not decouple yarn v1. Slowly improve that one.
- 99% of the options berry has make no sense to me. Overwhelmed by
  choices.

  Every configuration option is listed, with no help given to making the
  bread and butter scenarios prominent.

  My guess is the settings are to fill niche caches and specialized
  project layouts.

  What would help:

  Show practical examples of what type of projects these configuration
  settings are for. If it's not fitting the scenario - don't even
  mention it.
- Not a fan of how YN0000 shows everywhere.
  yarnpkg/berry#2517 since Feb 25, 2021

  This may have a practical use case for it, e.g. when certain
  operations run in parallel. But this shows even when an operation
  isn't parallel. Eek.
- The underlying feeling I have is berry is optimizing for the maintainers,
  and the user is an afterthought.

  This is direct opposite to what yarn is about. Yarn is about hiding
  complexity, user convenience and happiness, and performance.

  As odd as that may sound - it's important to strike a balance of a
  good product and a good developer experience.
- I still see a lot of potential and benefit in berry

  As an open source maintainer

  The architecture is there, it just needs to get polished to show what
  matters to the user, to convey its value.
tony added a commit to tony/cv that referenced this issue Sep 5, 2022
Even though we may not ultimately use it:

- It seems to be a project in the midst of a philosophical problem.
- Why give it another name, unless losing track of where it came from
- It seems to decouple architecture (yay) but I'm concerned
  with not adopting an incremental approach to make yarn v1 better.

  Why not decouple yarn v1. Slowly improve that one.
- 99% of the options berry has make no sense to me. Overwhelmed by
  choices.

  Every configuration option is listed, with no help given to making the
  bread and butter scenarios prominent.

  My guess is the settings are to fill niche caches and specialized
  project layouts.

  What would help:

  Show practical examples of what type of projects these configuration
  settings are for. If it's not fitting the scenario - don't even
  mention it.
- Not a fan of how YN0000 shows everywhere.
  yarnpkg/berry#2517 since Feb 25, 2021

  This may have a practical use case for it, e.g. when certain
  operations run in parallel. But this shows even when an operation
  isn't parallel. Eek.
- The underlying feeling I have is berry is optimizing for the maintainers,
  and the user is an afterthought.

  This is direct opposite to what yarn is about. Yarn is about hiding
  complexity, user convenience and happiness, and performance.

  As odd as that may sound - it's important to strike a balance of a
  good product and a good developer experience.
- I still see a lot of potential and benefit in berry

  As an open source maintainer

  The architecture is there, it just needs to get polished to show what
  matters to the user, to convey its value.
tony added a commit to tony/cv that referenced this issue Sep 5, 2022
Even though we may not ultimately use it:

- It seems to be a project in the midst of a philosophical problem.
- Why give it another name, unless losing track of where it came from
- It seems to decouple architecture (yay) but I'm concerned
  with not adopting an incremental approach to make yarn v1 better.

  Why not decouple yarn v1. Slowly improve that one.
- 99% of the options berry has make no sense to me. Overwhelmed by
  choices.

  Every configuration option is listed, with no help given to making the
  bread and butter scenarios prominent.

  My guess is the settings are to fill niché cases and specialized
  project layouts.

  What would help:

  Show practical examples of what type of projects these configuration
  settings are for. If it's not fitting the scenario - don't even
  mention it.
- Not a fan of how YN0000 shows everywhere.
  yarnpkg/berry#2517 since Feb 25, 2021

  This may have a practical use case for it, e.g. when certain
  operations run in parallel. But this shows even when an operation
  isn't parallel. Eek.
- The underlying feeling I have is berry is optimizing for the maintainers,
  and the user is an afterthought.

  This is direct opposite to what yarn is about. Yarn is about hiding
  complexity, user convenience and happiness, and performance.

  As odd as that may sound - it's important to strike a balance of a
  good product and a good developer experience.
- I still see a lot of potential and benefit in berry

  As an open source maintainer

  The architecture is there, it just needs to get polished to show what
  matters to the user, to convey its value.
@qrohlf
Copy link
Contributor

qrohlf commented Dec 19, 2022

I would vote against closing this issue, as even with enableMessageNames set to false, the arrow is still printed; this breaks the regex that Github Actions uses for parsing type errors and results in bad CI behavior when running ci tasks via foreach.

For now I'm able to use YARN_ENABLE_MESSAGE_NAMES=false yarn workspaces foreach run typecheck | cut -c 2- to work around this. In the future I'd like to potentially spin up a PR to add a flag which would make foreach emit raw output. Would --raw-output be a reasonable thing to name that flag?

@sargunv
Copy link
Contributor Author

sargunv commented Dec 20, 2022

Should enableMessageNames=false simply stop printing the arrow?

@qrohlf
Copy link
Contributor

qrohlf commented Dec 21, 2022

That seems like a reasonable approach, too.

I honestly feel like the default should be to omit the arrow and YN0000 prefix. Vanilla yarn exec is one of the few yarn commands that does not prefix its output like this, which is good because it means that the output is compatible with things like like the Github Actions tsc matcher and similar tools that parse CLI output. yarn foreach exec breaks this, because of the additional prefixing in its output. Right now, I'm working around those limitations with patched, more permissive versions of the regexes, but that's not exactly ideal.

@qrohlf
Copy link
Contributor

qrohlf commented Dec 21, 2022

Actually, re-reading the scrollback it seems like @arcanis agrees with me that the ➤ YN0000: output should be removed from foreach entirely. I'll see about spinning up a PR to do this, as that seems like the best and simplest solution to this issue.

@qrohlf
Copy link
Contributor

qrohlf commented Dec 21, 2022

I have started a PR to address this over at #5152. I'd love if a maintainer can take a look at that diff (it's very short, 15 lines of code) and let me know how they feel about the general approach taken; then I will set about updating the test suite.

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

Successfully merging a pull request may close this issue.

8 participants