Skip to content

Commit bfc32bd

Browse files
authored
fix(ext/node): stop coloring non-TTY streams in util.styleText (#35236)
Fixes #35203. <img width="522" height="152" alt="grafik" src="https://github.com/user-attachments/assets/cc2b9611-16b0-46f7-bffb-9c60123c7083" /> None of this was done using any LLMs but I briefly looked at the diff of #35204 to see the relevant files so if the scope of the fix is broader and the LLM missed it then I may have made the same mistake. <!-- IMPORTANT: If you used AI tools (e.g. Copilot, ChatGPT, Claude, Cursor, etc.) to help write this PR, you MUST disclose it in the PR description. PRs will be rejected if there is suspicion of undisclosed AI usage. Before submitting a PR, please read https://docs.deno.com/runtime/manual/references/contributing 1. Give the PR a descriptive title. Examples of good title: - fix(ext/net): fix race condition in TCP listener - docs(runtime): update permissions API docstrings - feat(cli): add --env-file flag to `deno run` - refactor(ext/node): simplify Buffer.from() implementation - test(ext/fs): add spec tests for symlink resolution Examples of bad title: - fix #7123 - update docs - fix bugs 2. Ensure there is a related issue and it is referenced in the PR text. 3. Ensure there are tests that cover the changes. 4. Ensure `./x fmt` passes without changing files. 5. Ensure `./x lint` passes. 6. If you used AI tools to help write this PR, you MUST disclose it below. PRs will be rejected if there is suspicion of undisclosed AI usage. 7. Open as a draft PR if your work is still in progress. The CI won't run all steps, but you can add '[ci]' to a commit message to force it to. 8. If you would like to run the benchmarks on the CI, add the 'ci-bench' label. -->
1 parent b40113d commit bfc32bd

6 files changed

Lines changed: 164 additions & 51 deletions

File tree

ext/node/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -614,6 +614,7 @@ deno_core::extension!(deno_node,
614614
"internal_binding/constants.ts",
615615
"internal_binding/_libuv_winerror.ts",
616616
"internal_binding/uv.ts",
617+
"internal/util/colorize.mjs",
617618
"internal/util/inspect.mjs",
618619
"internal/errors.ts",
619620
"internal/errors/error_source.ts",

ext/node/polyfills/internal/util.mjs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ const { os } = core.loadExtScript(
1818
const { validateFunction } = core.loadExtScript(
1919
"ext:deno_node/internal/validators.mjs",
2020
);
21+
const { shouldColorize } = core.loadExtScript(
22+
"ext:deno_node/internal/util/colorize.mjs",
23+
);
2124
const { isNativeError } = core.loadExtScript(
2225
"ext:deno_node/internal/util/types.ts",
2326
);
@@ -295,6 +298,7 @@ return {
295298
pendingDeprecate,
296299
promisify,
297300
removeColors,
301+
shouldColorize,
298302
sleep,
299303
WeakReference,
300304
default: {
@@ -311,6 +315,7 @@ return {
311315
pendingDeprecate,
312316
promisify,
313317
removeColors,
318+
shouldColorize,
314319
sleep,
315320
},
316321
};
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
// Copyright 2018-2026 the Deno authors. MIT license.
2+
3+
(function () {
4+
const { primordials } = __bootstrap;
5+
const { String } = primordials;
6+
7+
// Mirrors Node's internal/util/colors.shouldColorize so node_compat tests
8+
// that flip FORCE_COLOR / NODE_DISABLE_COLORS / NO_COLOR / TERM=dumb behave
9+
// like Node. NO_COLOR / NODE_DISABLE_COLORS / TERM=dumb only apply via
10+
// `stream.getColorDepth()` (matching Node's tty.WriteStream.getColorDepth)
11+
// or under FORCE_COLOR; a non-TTY plain stream stays at `false`, while a
12+
// fake stream with `isTTY=true` and no `getColorDepth` returns `true`.
13+
function shouldColorize(stream) {
14+
const env = globalThis.process.env || {};
15+
if (env.FORCE_COLOR !== undefined) {
16+
const v = String(env.FORCE_COLOR);
17+
if (v === "0" || v === "false") return false;
18+
if (
19+
(env.NODE_DISABLE_COLORS !== undefined &&
20+
env.NODE_DISABLE_COLORS !== "") ||
21+
(env.NO_COLOR !== undefined && env.NO_COLOR !== "") ||
22+
env.TERM === "dumb"
23+
) {
24+
// FORCE_COLOR with any non-default value still wins in Node 20+.
25+
return v !== "" && v !== "0";
26+
}
27+
return true;
28+
}
29+
if (!stream?.isTTY) return false;
30+
if (typeof stream.getColorDepth === "function") {
31+
return stream.getColorDepth() > 2;
32+
}
33+
// Match Node: TTY-flagged stream with no getColorDepth defaults to colorful.
34+
return true;
35+
}
36+
37+
return { shouldColorize };
38+
})();

ext/node/polyfills/internal/util/inspect.mjs

Lines changed: 40 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,9 @@ const {
6161
validateString,
6262
} = core.loadExtScript("ext:deno_node/internal/validators.mjs");
6363
const { codes } = core.loadExtScript("ext:deno_node/internal/error_codes.ts");
64+
const { shouldColorize } = core.loadExtScript(
65+
"ext:deno_node/internal/util/colorize.mjs",
66+
);
6467
const {
6568
colors,
6669
createStylizeWithColor,
@@ -658,27 +661,49 @@ function styleText(format, text, options) {
658661
const validateStream = options?.validateStream ?? true;
659662
validateBoolean(validateStream, "options.validateStream");
660663

664+
// Match node: validate stream before any format validation, so any
665+
// ERR_INVALID_ARG_TYPE always gets throws regardless of
666+
// `ERR_INVALID_ARG_VALUE` errors.
661667
if (validateStream) {
662668
const stream = options?.stream;
663-
if (
664-
stream !== undefined && stream !== null &&
665-
// Match Node's lib/util.js: bail when `stream` is not a Readable/
666-
// Writable/Node Stream. Approximate the duck-type used in those checks.
667-
!(typeof stream === "object" && (
668-
typeof stream.read === "function" ||
669-
typeof stream.write === "function" ||
670-
typeof stream.pipe === "function"
671-
))
672-
) {
673-
throw new codes.ERR_INVALID_ARG_TYPE(
674-
"stream",
675-
["ReadableStream", "WritableStream", "Stream"],
676-
stream,
677-
);
669+
if (stream !== undefined && stream !== null) {
670+
// Match Node: bail when `stream` is not a Readable/Writable/Node Stream.
671+
// Approximate the duck-type used in those checks.
672+
if (
673+
!(typeof stream === "object" && (
674+
typeof stream.read === "function" ||
675+
typeof stream.write === "function" ||
676+
typeof stream.pipe === "function"
677+
))
678+
) {
679+
throw new codes.ERR_INVALID_ARG_TYPE(
680+
"stream",
681+
["ReadableStream", "WritableStream", "Stream"],
682+
stream,
683+
);
684+
}
678685
}
679686
}
680687

688+
// Match Node: validate format even when colorizing is skipped, so
689+
// ERR_INVALID_ARG_VALUE is always thrown regardless of TTY state.
681690
const formatArray = ArrayIsArray(format) ? format : [format];
691+
for (let i = 0; i < formatArray.length; i++) {
692+
const key = formatArray[i];
693+
if (key === "none") continue;
694+
if (inspect.colors[key] == null) {
695+
validateOneOf(key, "format", ObjectKeys(inspect.colors));
696+
}
697+
}
698+
699+
if (validateStream) {
700+
// Match Node defaulting stream to process.stdout: when no stream is
701+
// provided the colorize check still runs against stdout.
702+
const effectiveStream = options?.stream ?? globalThis.process?.stdout;
703+
if (!shouldColorize(effectiveStream)) {
704+
return text;
705+
}
706+
}
682707

683708
let openCodes = "";
684709
let closeCodes = "";
@@ -689,9 +714,6 @@ function styleText(format, text, options) {
689714
if (key === "none") continue;
690715

691716
const formatCodes = inspect.colors[key];
692-
if (formatCodes == null) {
693-
validateOneOf(key, "format", ObjectKeys(inspect.colors));
694-
}
695717
const openNum = formatCodes[0];
696718
const closeNum = formatCodes[1];
697719
const openSeq = kStyleTextEscape + openNum + kStyleTextEscapeEnd;

ext/node/polyfills/repl.ts

Lines changed: 4 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,9 @@ const {
7474
const { validateFunction } = core.loadExtScript(
7575
"ext:deno_node/internal/validators.mjs",
7676
);
77+
const { shouldColorize } = core.loadExtScript(
78+
"ext:deno_node/internal/util.mjs",
79+
);
7780
const vm = core.loadExtScript("ext:deno_node/vm.js").default;
7881
import process from "node:process";
7982
import path from "node:path";
@@ -194,36 +197,6 @@ function _removeNewListenerGuard() {
194197
}
195198
}
196199

197-
// Mirrors Node's internal/util/colors.shouldColorize so node_compat tests
198-
// that flip FORCE_COLOR / NODE_DISABLE_COLORS / NO_COLOR / TERM=dumb behave
199-
// like Node. NO_COLOR / NODE_DISABLE_COLORS / TERM=dumb only apply via
200-
// `stream.getColorDepth()` (matching Node's tty.WriteStream.getColorDepth)
201-
// or under FORCE_COLOR; a non-TTY plain stream stays at `false`, while a
202-
// fake stream with `isTTY=true` and no `getColorDepth` returns `true`.
203-
function _shouldColorize(stream: any): boolean {
204-
const env = (process as any).env || {};
205-
if (env.FORCE_COLOR !== undefined) {
206-
const v = String(env.FORCE_COLOR);
207-
if (v === "0" || v === "false") return false;
208-
if (
209-
(env.NODE_DISABLE_COLORS !== undefined &&
210-
env.NODE_DISABLE_COLORS !== "") ||
211-
(env.NO_COLOR !== undefined && env.NO_COLOR !== "") ||
212-
env.TERM === "dumb"
213-
) {
214-
// FORCE_COLOR with any non-default value still wins in Node 20+.
215-
return v !== "" && v !== "0";
216-
}
217-
return true;
218-
}
219-
if (!stream?.isTTY) return false;
220-
if (typeof stream.getColorDepth === "function") {
221-
return stream.getColorDepth() > 2;
222-
}
223-
// Match Node: TTY-flagged stream with no getColorDepth defaults to colorful.
224-
return true;
225-
}
226-
227200
const reAtFrame = new SafeRegExp(/^\s+at\s/);
228201
const reExtFrame = new SafeRegExp(/\bext:[a-z_]+\//);
229202
const reNodeParenFrame = new SafeRegExp(/\(node:[a-z_]+:/);
@@ -668,7 +641,7 @@ export class REPLServer extends (Interface as any) {
668641
const usePreview = previewSession !== null;
669642

670643
if (options.terminal && options.useColors === undefined) {
671-
options.useColors = _shouldColorize(options.output);
644+
options.useColors = shouldColorize(options.output);
672645
}
673646

674647
// Default prompt

tests/unit_node/util_test.ts

Lines changed: 76 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { stripAnsiCode } from "@std/fmt/colors";
1010
import * as util from "node:util";
1111
import utilDefault from "node:util";
1212
import { Buffer } from "node:buffer";
13+
import process from "node:process";
1314

1415
Deno.test({
1516
name: "[util] format",
@@ -262,15 +263,88 @@ Deno.test("[util] aborted() drops pending promise when resource is GCed", async
262263
});
263264

264265
Deno.test("[util] styleText()", () => {
265-
const redText = util.styleText("red", "error");
266+
const redText = util.styleText("red", "error", { validateStream: false });
266267
assertEquals(redText, "\x1B[31merror\x1B[39m");
267268
});
268269

269270
Deno.test("[util] styleText() with array of formats", () => {
270-
const colored = util.styleText(["red", "green"], "error");
271+
const colored = util.styleText(["red", "green"], "error", {
272+
validateStream: false,
273+
});
271274
assertEquals(colored, "\x1b[31m\x1b[32merror\x1b[39m\x1b[39m");
272275
});
273276

277+
Deno.test("[util] styleText() respects stream.isTTY", () => {
278+
const streamTTY = {
279+
write() {},
280+
isTTY: true,
281+
} as unknown as NodeJS.WritableStream;
282+
const streamNoTTY = {
283+
write() {},
284+
isTTY: false,
285+
} as unknown as NodeJS.WritableStream;
286+
287+
const redText = util.styleText("red", "TTY", { stream: streamTTY });
288+
assertEquals(redText, "\x1b[31mTTY\x1b[39m");
289+
290+
const plainText = util.styleText("blue", "No TTY", { stream: streamNoTTY });
291+
const greenText = util.styleText("green", "No TTY", {
292+
stream: streamNoTTY,
293+
validateStream: false,
294+
});
295+
assertEquals(plainText, "No TTY");
296+
assertEquals(greenText, "\x1b[32mNo TTY\x1b[39m");
297+
});
298+
299+
Deno.test("[util] styleText() falls back to process.stdout when no stream given", () => {
300+
const orig = process.env.FORCE_COLOR;
301+
try {
302+
process.env.FORCE_COLOR = "0";
303+
assertEquals(util.styleText("red", "no stream"), "no stream");
304+
305+
process.env.FORCE_COLOR = "1";
306+
assertEquals(
307+
util.styleText("red", "no stream"),
308+
"\x1b[31mno stream\x1b[39m",
309+
);
310+
} finally {
311+
if (orig === undefined) {
312+
delete process.env.FORCE_COLOR;
313+
} else {
314+
process.env.FORCE_COLOR = orig;
315+
}
316+
}
317+
});
318+
319+
Deno.test("[util] styleText() validates stream type before format", () => {
320+
// When both the stream and the format are invalid, ERR_INVALID_ARG_TYPE
321+
// (bad stream) must be thrown before ERR_INVALID_ARG_VALUE (bad format),
322+
// matching Node's argument-validation order.
323+
const streamErr = assertThrows(
324+
() =>
325+
// @ts-expect-error: intentionally invalid format and stream to test validation order
326+
util.styleText("not_a_valid_format", "text", { stream: "not-a-stream" }),
327+
TypeError,
328+
);
329+
assertEquals(
330+
(streamErr as unknown as { code: string }).code,
331+
"ERR_INVALID_ARG_TYPE",
332+
);
333+
334+
// With a structurally-valid stream (has .write), the format check fires next
335+
// and ERR_INVALID_ARG_VALUE is thrown.
336+
const formatErr = assertThrows(
337+
() =>
338+
// @ts-expect-error: intentionally invalid format to test validation order
339+
util.styleText("not_a_valid_format", "text", { stream: { write() {} } }),
340+
TypeError,
341+
);
342+
assertEquals(
343+
(formatErr as unknown as { code: string }).code,
344+
"ERR_INVALID_ARG_VALUE",
345+
);
346+
});
347+
274348
Deno.test("[util] stripVTControlCharacters() removes OSC 8 hyperlinks", () => {
275349
// OSC 8 hyperlink with ESC \ (ST) terminator
276350
const input =

0 commit comments

Comments
 (0)