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

escape codes and emojis when redirecting stderr to file #1201

Closed
kzc opened this issue Dec 29, 2016 · 13 comments · Fixed by #1206
Closed

escape codes and emojis when redirecting stderr to file #1201

kzc opened this issue Dec 29, 2016 · 13 comments · Fixed by #1206

Comments

@kzc
Copy link
Contributor

kzc commented Dec 29, 2016

Would it be possible to automatically disable chalk escape codes and emojis if stderr is a is not a tty? (correction)

rollup version 0.39.0

$ echo "" > empty.js

$ node bin/rollup empty.js 2> err.txt

$ od -c err.txt
0000000  342 232 240   ️  **  **             033   [   1   m   G   e   n
0000020    e   r   a   t   e   d       a   n       e   m   p   t   y    
0000040    b   u   n   d   l   e 033   [   2   2   m  \n  \n            
0000055
@Rich-Harris
Copy link
Contributor

Ha, whoops – didn't anticipate that scenario. Is this just a case of doing something like this?

const stderr = process.stderr.isTTY ?
  console.error.bind( console ) :
  str => console.error( stripControlCodesSomehow( str ) );

If so, any idea what stripControlCodesSomehow looks like?

@kzc
Copy link
Contributor Author

kzc commented Dec 30, 2016

If so, any idea what stripControlCodesSomehow looks like?

I think it's better to not emit these codes rather than strip them after the fact.

Only a single emoji was introduced as far as I can tell:

stderr( `⚠️ ${chalk.bold( warning.message )}` );

rather than strip it, that need not be emitted if stderr is is not a TTY. (correction)

I never used chalk before but I assume they must have a mode to not emit colors and control characters. If not, a wrapper could be made for chalk (red, grey, cyan, bold, dim, etc) to just be string passthroughs.

@kzc
Copy link
Contributor Author

kzc commented Dec 30, 2016

Although I think a passthrough chalk wrapper is the correct way to go, this regexp purports to remove terminal escape codes:

http://stackoverflow.com/questions/25245716/remove-all-ansi-colors-styles-from-strings/29497680#29497680

@kzc
Copy link
Contributor Author

kzc commented Dec 30, 2016

@kzc
Copy link
Contributor Author

kzc commented Dec 30, 2016

This patch fixes the redirected stderr issue. However, it uses require(). For the life of me I can't figure out how to get import to work with the mutable chalk singleton - Illegal reassignment to import 'chalk'?

diff --git a/bin/src/handleError.js b/bin/src/handleError.js
index aff6cea..0d5dd10 100644
--- a/bin/src/handleError.js
+++ b/bin/src/handleError.js
@@ -1,4 +1,6 @@
-import * as chalk from 'chalk';
+const chalk = require( "chalk" );
+
+if ( !process.stderr.isTTY ) chalk.enabled = false;
 
 function stderr ( msg ) {
        console.error( msg ); // eslint-disable-line no-console
diff --git a/bin/src/runRollup.js b/bin/src/runRollup.js
index f511de1..549157c 100644
--- a/bin/src/runRollup.js
+++ b/bin/src/runRollup.js
@@ -1,7 +1,7 @@
 import { realpathSync } from 'fs';
 import * as rollup from 'rollup';
 import relative from 'require-relative';
-import * as chalk from 'chalk';
+const chalk = require( "chalk" );
 import handleError from './handleError';
 import relativeId from '../../src/utils/relativeId.js';
 import SOURCEMAPPING_URL from './sourceMappingUrl.js';
@@ -154,7 +154,8 @@ function execute ( options, command ) {
                        if ( seen.has( str ) ) return;
                        seen.add( str );
 
-                       stderr( `⚠️   ${chalk.bold( warning.message )}` );
+                       const warnSymbol = process.stderr.isTTY ? `⚠️   ` : `Warning: `;
+                       stderr( `${warnSymbol}${chalk.bold( warning.message )}` );
 
                        if ( warning.url ) {
                                stderr( chalk.cyan( warning.url ) );

Amusing side note: I thought there was an escape code issue with emitting the dash in the warning (bytes 0xe2 0x80 0x93) until I learned that some rollup warnings use the Unicode Character 'EN DASH' (U+2013) instead of the typical 'HYPHEN-MINUS' (U+002D). Was not expecting that.

@Rich-Harris
Copy link
Contributor

Thanks, have turned into a PR (#1206). The chalk thing can be fixed by doing...

import chalk from 'chalk';

...instead of...

import * as chalk from 'chalk';

...the end result is the same, except that Rollup doesn't think you're reassigning stuff illegally.

Amusing side note: I thought there was an escape code issue with emitting the dash in the warning (bytes 0xe2 0x80 0x93) until I learned that rollup warnings use the Unicode Character 'EN DASH' (U+2013) instead of the typical 'HYPHEN-MINUS' (U+002D). Was not expecting that.

Ha, that's what happens when you work in publishing I suppose — you obsess over the correct punctuation. Though I thought I'd been typing an EM DASH all these years, not an EN DASH (which isn't the correct mark). Oops! Will have to retrain my fingers.

@kzc
Copy link
Contributor Author

kzc commented Dec 30, 2016

Thanks.

So import chalk from 'chalk'; is mutable - I learn something new every day!

Rich-Harris added a commit that referenced this issue Dec 30, 2016
apply kzc patch to prevent escape codes and emojis appearing in non-TTY stderr
@braytak
Copy link

braytak commented Aug 13, 2018

@kzc chalk.supportsColor.level will be 0 when you redirect to a file (or any destination that chalk deems as not color-capable).

Try this:

const chalk=require('chalk');
const util=require('util');
const o = {b: true, a:[1,2,3], s: 'string'}
console.log(util.inspect(o, {color: chalk.supportsColor.level !== 0}))

@kzc
Copy link
Contributor Author

kzc commented Aug 13, 2018

chalk.supportsColor.level will be 0 when you redirect to a file (or any destination that chalk deems as not color-capable).

@braytak It's been a while since I looked at this but the problem was not only a color issue. I didn't want emojis in the output (interfering with analysis) if it was not a TTY, hence the need for an explicit check.

I don't know the state of Rollup output now. It's probably changed and they may have reintroduced emojis unconditionally.

@shellscape
Copy link
Contributor

@kzc I'm interested to learn more about your environment, given that utf-8 support is pretty widespread. Could you share some more with us?

@kzc
Copy link
Contributor Author

kzc commented Aug 13, 2018

@shellscape I am an old school programmer and loathe emojis. They interfere with analysis. There ought to be a way to disable them. You were looking for input in #2393, so I brought this up.

@kzc
Copy link
Contributor Author

kzc commented Aug 13, 2018

Just to follow up, an option to disable emojis is not uncommon in other projects:

yarnpkg/yarn#3660 (comment)

@shellscape
Copy link
Contributor

Ah, I see. In that case, you should be able to use bash to accomplish that and still pipe to your desired destination:

sed 's/[^[:alnum:][:punct:][:space:]]//g' input

You might also be able wrap https://www.npmjs.com/package/emoji-strip in a CLI. I'm not sure that asking a library or app to filter certain unicode characters is reasonable solely based on personal preference, especially when a number of workarounds exist. And I'd question yarn's decision to add that option if I had any skin in the game.

Emoji were added in the Unicode Standard in version 6.0 that was released in 2010. I'd bet you're going to be progressively fighting an uphill battle as their use becomes more widespread. I'm just one maintainer though, and others may have different opinions.

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 a pull request may close this issue.

4 participants