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(workspace-tools): remove message prefix from workspaces foreach #5152

Merged
merged 7 commits into from
Dec 29, 2022
Merged

fix(workspace-tools): remove message prefix from workspaces foreach #5152

merged 7 commits into from
Dec 29, 2022

Conversation

qrohlf
Copy link
Contributor

@qrohlf qrohlf commented Dec 21, 2022

What's the problem this PR addresses?

problem statement taken from #2517

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

Closes #2517

How did you fix it?

Based on @arcanis's comment, I chose to remove the ➤ YN0000: prefix from the yarn workspaces foreach stream reporter output entirely, by adding an additional flag to the StreamReport options to allow suppressing the and message name.

This aligns with the existing behavior of the analogous single-workspace yarn exec command, which is one of the few Yarn Berry commands that does not add any kind of prefixing/formatting on top of its output.

Note that this is an unfamiliar codebase for me and I'm not sure if my code changes are the cleanest way of attaining the desired result here. I'm pushing this up without updating any of the tests yet to get feedback on this approach - very open to alternative suggestions.

This only applies to the outer layer of execution; if the enableMessageNames config option is set to true, the sub-executions of yarn workspaces foreach will still include the YNXXXX output.

Checklist

  • I have read the Contributing Guide.
  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@qrohlf qrohlf changed the title [STRAWMAN] Remove ➤ YN0000: output in yarn workspaces foreach Remove ➤ YN0000: output in yarn workspaces foreach Dec 21, 2022
@merceyz merceyz changed the title Remove ➤ YN0000: output in yarn workspaces foreach fix(workspace-tools): remove ➤ YN0000: prefix from workspaces foreach Dec 21, 2022
@merceyz merceyz changed the title fix(workspace-tools): remove ➤ YN0000: prefix from workspaces foreach fix(workspace-tools): remove message prefix from workspaces foreach Dec 21, 2022
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

@qrohlf
Copy link
Contributor Author

qrohlf commented Dec 21, 2022

@merceyz alternatively your earlier suggestion to use a ThrowReport instead of a StreamReport seems like it could also work.

Again, I'm not super familiar with this codebase so I'd be curious to know if there's a downside to swapping out StreamReport with ThrowReport within foreach that made you change your suggestion?

@merceyz
Copy link
Member

merceyz commented Dec 21, 2022

I'd be curious to know if there's a downside to swapping out StreamReport with ThrowReport within foreach that made you change your suggestion?

After checking the code a bit more I saw that it was easier to modify StreamReport so I essentially answered my own question.

@qrohlf qrohlf requested a review from merceyz December 22, 2022 16:32
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

includePrefix should apply to all the methods that print a prefix, not just reportInfo.

.yarn/versions/4aa0961e.yml Show resolved Hide resolved
.yarn/versions/4aa0961e.yml Outdated Show resolved Hide resolved
@qrohlf qrohlf requested a review from merceyz December 26, 2022 21:28
@qrohlf
Copy link
Contributor Author

qrohlf commented Dec 26, 2022

@merceyz PR updated - I went ahead and extracted the formatUtils.pretty and whitespace concatenation that was happening with the caret in all those reportFoo methods to a new private function as well, seemed like there was enough duplication happening in this file that it was worth it.

@merceyz merceyz enabled auto-merge (squash) December 29, 2022 14:42
@merceyz merceyz merged commit e479869 into yarnpkg:master Dec 29, 2022
@merceyz
Copy link
Member

merceyz commented Dec 29, 2022

Thanks for the fix!

merceyz added a commit that referenced this pull request Jan 29, 2023
…#5152)

* remove caret

* disable message names

* add version bump

* switch to includePrefix

* add test, update test snapshots

* apply includePrefix logic to all functions in StreamReport that write to the terminal

* refactor: rename to `formatPrefix` and move `formatIndent` into it

Co-authored-by: merceyz <merceyz@users.noreply.github.com>
@lusarz
Copy link

lusarz commented Feb 16, 2023

When it will be released?

@merceyz
Copy link
Member

merceyz commented Feb 16, 2023

It was released in v3.4.0

@qrohlf qrohlf deleted the workspaces-foreach-remove-caret branch February 17, 2023 04:22
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.

[Feature] Flag to suppress ➤ YN0000: output in yarn workspaces foreach
3 participants