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

Recursive Runs with Multiple Packages Lose Color #2148

Closed
shellscape opened this issue Nov 11, 2019 · 22 comments
Closed

Recursive Runs with Multiple Packages Lose Color #2148

shellscape opened this issue Nov 11, 2019 · 22 comments
Labels
area: lifecycle-scripts area: monorepo Everything related to the pnpm workspace feature

Comments

@shellscape
Copy link
Contributor

shellscape commented Nov 11, 2019

pnpm version: 4.2.2

Code to reproduce the issue:

Run a recursive command which outputs text with ansi colors stdio. For example, using rollup and ava:

$ pnpm run test --filter ./packages/strip --filter ./packages/replace

Expected behavior:

Expectation is that there exists a means to disable the muting of colors by pnpm and allowing the intended colors and ansi codes to be displayed.

Actual behavior:

The image below demonstrates what actually occurs.

Imgur

As you can see above, rollups output has been "muted" and greyed. While the image below shows the output from pnpm run build --filter ./packages/strip. Note that colors are correct when only one package is run against.

Imgur

Additional information:

  • node -v prints: 12.3.1
  • Windows, OS X, or Linux?: Mac OS
@panoply
Copy link

panoply commented Jun 2, 2020

Bumping this.

It would be great if when passing the --color flag via an pnpm command that colours will be inherited/respected, as of now --color will just print white.

I might be wrong, but I do recall reading somewhere that pnpm intentionally mutes/strips stdout when running recursively, don't hold me to that because I may have it confused with another project. Eitherway, I would love to see this.

@zkochan zkochan added the area: monorepo Everything related to the pnpm workspace feature label Jun 2, 2020
@zkochan
Copy link
Member

zkochan commented Jun 2, 2020

We can make the child processes inherit the parent's stdio

const stdio = (
opts.workspaceConcurrency === 1 ||
packageChunks.length === 1 && packageChunks[0].length === 1
) ? 'inherit' : 'pipe'

But it will be a mess. Is that ok?

@panoply
Copy link

panoply commented Jun 2, 2020

But it will be a mess. Is that ok?

So the pnpm output text when running recursive commands would be sacrificed? ie: we loose path prefix?

@zkochan
Copy link
Member

zkochan commented Jun 3, 2020

correct

@panoply
Copy link

panoply commented Jun 3, 2020

I've only partially explored the code base, at what point does pnpm strip the piped stdio when generating the recursive output? I see that chalk and supports-color is used, can we not leverage the environment variable? In my opinion, the way in which pnpm facilitates logging is a huge part of its aesthetic and the overall pnpm feel (so-to-speak).

@panoply
Copy link

panoply commented Jun 3, 2020

For more context, opposed to inheriting, when the --color flag is detected, can we not parse the piped stdio and remove any characters that would cause conflict (eg: (\n\s{0,1}|\-{5,}) while preserving newlines and from here split out and appropriate?

@rhyek
Copy link

rhyek commented Nov 6, 2020

lerna seems to preserve colors with --parallel and also shows a prefix:

image

@zkochan
Copy link
Member

zkochan commented Nov 6, 2020

if Lerna managed to do it, then we can probably as well.

@zkochan zkochan added this to the v5.11 milestone Nov 6, 2020
@zkochan zkochan modified the milestones: v5.11, v5.x Nov 15, 2020
@rhyek
Copy link

rhyek commented Jan 25, 2021

Any idea when this might get looked at? With microsoft/TypeScript#40584 fixed this is the only issue I'm waiting for to switch over from lerna.

@zkochan zkochan modified the milestones: v5.x, v6.x Mar 23, 2021
@panoply
Copy link

panoply commented Aug 21, 2021

At what point is pnpm stripping the stdio? This one is becoming a little difficult to wrangle lately, which module handles this action?

@ghostdevv
Copy link

Does anyone have a solution to this?

@rhyek
Copy link

rhyek commented Nov 5, 2021

Btw, I was wrong about Lerna handling this correctly. In general, it is my understanding that programs will internally decide wether to output color escape codes and usually they do so by detecting if they are currently attached to a tty. If not, they do not output the codes. Pnpm and lerna will pipe stdout of programs they manage for us so that they can prefix (with color) their output for our benefit. Doing so means the programs will not be running inside a tty directly.

Some programs allow for forcing their output via some environment variable or command line argument. For example, node programs that use https://www.npmjs.com/package/chalk can set FORCE_COLOR=1.

I don't think a general solution applicable to all programs regardless of their support for flags is something pnpm (or lerna) can easily or should do. I might be wrong about this, but I think they would have to run some sort of pseudo terminal (maybe https://github.com/microsoft/node-pty?) per program so they think they're inside a tty, capture their output, and finally output that with the colored prefixes to the real tty.

@zheeeng
Copy link

zheeeng commented Feb 18, 2022

I'm using npm-run-all, it managed output color, can we try its solution?

@woubuc
Copy link
Contributor

woubuc commented Apr 12, 2022

So I've tracked down where the colour gets stripped because I ran into this issue and it is a little annoying to lose all colour output.

The short version is that ANSI codes are passed from the script output correctly to pnpm and are maintained all the way through the different logger steps. But right at the end, all ANSI codes are stripped while truncating output lines from lifecycle scripts in packages/default-reporter/src/reporterForClient/reportLifecycleScripts.ts:270. Removing the call to stripAnsi() there fixes the issue (but likely introduces other issues).

This should be replaced with a better solution that will truncate the line while being aware of ANSI escape codes, like for instance the cli-truncate package.

@sebaplaza
Copy link

Any advance on this ?

@zkochan
Copy link
Member

zkochan commented Oct 15, 2022

There was an attempt to make it work but it breaks the output: #4559 (comment)

@zkochan zkochan removed this from the v6.x milestone Oct 15, 2022
@DzmitryFil
Copy link

+1 very annoying

@pokey
Copy link

pokey commented Mar 12, 2023

Some programs allow for forcing their output via some environment variable or command line argument. For example, node programs that use https://www.npmjs.com/package/chalk can set FORCE_COLOR=1.

Is it not risky to run with FORCE_COLOR=1? I believe this environment variable would be inherited by all children of a script. What if a program invokes another program and pipes that output to a file? It would be incorrect / surprising to get color codes there

@rhyek's solution of giving each script its own pty using eg https://github.com/microsoft/node-pty seems like the safest bet. It seems like if pnpm detects that it is attached to a tty, then it should use ptys for each recursive process. But certainly possible I'm missing something here

I'm using npm-run-all, it managed output color, can we try its solution?

I haven't actually tested, but from the docs it looks like color only works when the output isn't prefixed

Maybe the simplest place to start is to have a flag to run that causes it to run scripts sequentially and just output a single line saying which script is running right before running each script? Then each script could just inherit the pty. That would solve the use case where I'm not interested in parallelism so much as an easy way to run a script on all my packages. But maybe such a flag already exists and I missed it?

@valerii15298
Copy link

+1 also having problems with colours

@peteromano
Copy link

Yea this issue really stagnated..

@FFdhorkin
Copy link

There appears to be a regression on this. I have mentioned it in #7228 , but it appears colors are once again being stripped. I was using 8.5.1 and upgraded to 8.9.2 but they're still being removed

@zkochan
Copy link
Member

zkochan commented Oct 21, 2023

I don't think it is a regression. The suggested solution never worked 100% correctly as I have posted in the comment: #4559 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: lifecycle-scripts area: monorepo Everything related to the pnpm workspace feature
Projects
None yet
Development

No branches or pull requests