[BUG] ts-node/register not compatible with Danger even when DANGER_DISABLE_TRANSPILATION=true

[justjake]

Describe the bug

Danger's built-in transpiler for Typescript produces syntax errors when it tries to build some code we import in dangerfile.ts. This bug report is not about danger's typescript transpiler. It's about struggling to find a work-around to a bug in danger's traspiler compiler.

Because Danger's built-in transpiler can't run our dangerfile, we want to use our standard TS_NODE_PROJECT="tsconfig.tsnode.json" node -r ts-node/register/transpile-only workflow with danger.
However, even when setting DANGER_DISABLE_TRANSPILATION=true, danger still doesn't pick up the custom require plugins registered by ts-node.

This is the command I'm running:

DEBUG=danger:* \
DANGER_DISABLE_TRANSPILATION=true \
  TS_NODE_PROJECT=tsconfig.tsnode.json \
  NODE_OPTIONS="-r ts-node/register/transpile-only" \
  node node_modules/.bin/danger local --dangerfile src/dangerfile.ts

It outputs as you'd expect for a .ts file run through danger without transpilation:

Unable to evaluate the Dangerfile
 src/dangerfile.ts:1
import { StatsWithChunkGroups } from "./cli/statsComparison"
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at wrapSafe (internal/modules/cjs/loader.js:1053:16)
    at Module._compile (internal/modules/cjs/loader.js:1101:27)
    at Object.requireFromString [as default] (/Users/jitl/src/notion/node_modules/require-from-string/index.js:28:4)
    at Object.<anonymous> (/Users/jitl/src/notion/node_modules/danger/source/runner/runners/inline.ts:103:38)
    at step (/Users/jitl/src/notion/node_modules/danger/distribution/runner/runners/inline.js:32:23)
    at Object.next (/Users/jitl/src/notion/node_modules/danger/distribution/runner/runners/inline.js:13:53)
    at /Users/jitl/src/notion/node_modules/danger/distribution/runner/runners/inline.js:7:71
    at new Promise (<anonymous>)
    at __awaiter (/Users/jitl/src/notion/node_modules/danger/distribution/runner/runners/inline.js:3:12)
    at Object.exports.runDangerfileEnvironment (/Users/jitl/src/notion/node_modules/danger/source/runner/runners/inline.ts:62:75)

I suspect this is happening because danger overrides require.extensions even when transpilation is supposed to be disabled here:

danger-js/source/runner/runners/inline.ts

Lines 67 to 82 in c4fa130

	const customModuleHandler = (module: any, filename: string) => {
	if (!filename.includes("node_modules")) {
	d("Handling custom module: ", filename)
	}
	const contents = fs.readFileSync(filename, "utf8")
	const compiled = compile(contents, filename)
	module._compile(compiled, filename)
	}
	const customRequire = moduleHandler || customModuleHandler
	// Tell all these filetypes to ge the custom compilation
	require.extensions[".ts"] = customRequire
	require.extensions[".tsx"] = customRequire
	require.extensions[".js"] = customRequire
	require.extensions[".jsx"] = customRequire

To Reproduce
Steps to reproduce the behavior:

1. Install danger and ts-node.
2. Write a dangerfile.ts file that does some imports
3. Try the above command pointing to your dangerfile.

Expected behavior
Danger should respect already-register require plugins when transpilation is disabled. I would expect the command posted to work without syntax errors.

software	version
danger.js	10.6.2
node	v12.18.3
npm	6.14.8
Operating System	macOS 10.15.7
ts-node	v7.0.1
typescript	v3.4.5

Additional context

To work around both this problem, and the original problem with Danger's typescript compiler producing syntax errors, I've decided to transpile dangerfile.ts using tsc as part of our normal build process. In that case, a different work-around is needed to avoid the transpiled typescript from erroring out due to importing the dummy danger node module at runtime. Here's the code snippet I'm employing:

// src/dangerfile.ts
import { StatsWithChunkGroups } from "./cli/statsComparison"

/**
 * We would like to write this code:
 *
 *   import { danger, fail, schedule, message, warn } from "./danger"
 *
 * However, that would make us rely on Danger's very basic internal transpiler,
 * which barfs on our actual code that we're trying to import from statsComparison.
 * So instead, we have to do this work-around so we can pre-build dangerfile.ts using
 * our normal server build process.
 *
 * Also remember that the "danger" module exists only for types - at runtime, importing
 * it throws an error, because instead danger puts all the DSL bits into global scope
 * directly.
 */
function dangerImport<T extends keyof typeof import("danger")>(name: T) {
	const value: typeof import("danger")[T] = (global as any)[name]
	return value
}

const danger = dangerImport("danger")
const fail = dangerImport("fail")
const schedule = dangerImport("schedule")
const message = dangerImport("message")
const warn = dangerImport("warn")

[orta]

Quick guess because I'm about go step into a meeting. I'm not certain the root cause will be those overrides (though I think it's a good guess) - it's possible that the "evaluation" sub-process doesn't know about ts-node at all ( e.g. the thing that powers this:: https://danger.systems/js/usage/danger-process.html )

e.g. maybe some of these flags need to go through this?

danger-js/source/commands/utils/dangerRunToRunnerCLI.ts

Lines 2 to 57 in 913db0e

	const usesProcessSeparationCommands = ["ci", "pr", "local"]
	const dangerRunToRunnerCLI = (argv: string[]) => {
	let newCommand = []
	newCommand.push(argv[0])
	// e.g. node --inspect distribution/commands/danger-run-ci.js --dangerfile myDangerfile.ts
	// or node distribution/commands/danger-pr.js --dangerfile myDangerfile.ts
	if (argv.length === 1) {
	return ["danger", "runner"]
	} else if (argv[0].includes("node") || process.pkg != null) {
	// convert
	let newJSFile = argv[1]
	usesProcessSeparationCommands.forEach(name => {
	newJSFile = newJSFile.replace("danger-" + name, "danger-runner")
	})
	// Support re-routing internally in npx for danger-ts
	// If I recall, npm 7 is getting an npx re-write, so it might
	// be worth recommending yarn, but that requires folks using yarn 2
	// which I'm not sure will ever get the same level of a adoption of yarn v1
	//
	if (newJSFile.includes("npx") && newJSFile.endsWith("danger-ts")) {
	newJSFile = join(
	newJSFile,
	"..",
	"..",
	"lib",
	"node_modules",
	"danger-ts",
	"node_modules",
	"danger",
	"distribution",
	"commands",
	"danger-runner.js"
	)
	}
	newCommand.push(newJSFile)
	for (let index = 2; index < argv.length; index++) {
	newCommand.push(argv[index])
	}
	} else {
	// e.g. danger ci --dangerfile
	// if you do `danger run` start looking at args later
	newCommand.push("runner")
	let index = usesProcessSeparationCommands.includes(argv[1]) ? 2 : 1
	for (; index < argv.length; index++) {
	newCommand.push(argv[index])
	}
	}
	return newCommand
	}

[justjake]

@orta also off the cuff because I'm about to head to a meeting, setting NODE_OPTIONS="-r ts-node/register/transpile-only" in the environment should be enough to ensure that all child node processes require ts-node/regsiter/transpile-only because those flags are env environment variables, not being passed directly as argv to the child process; I would expect the only thing that would break the NODE_OPTIONS is if you clear out the environment used by the subprocess.

My two theories are (1) the overriding of require.extensions as i shared originally, and (2) maybe it would work, but perhaps dangerfile.ts isn't actually require'd, so no matter what it wouldn't get the benefit of ts-node (?) but i haven't investigated (2) yet.

I did want to give y'all an example of this problem and a workaround; so even if this issue gets closed WONTFIX TOO WEIRD, the next person googling "tsconfig ts-node dangerfile syntax error" will find my workaround :).

[orta]

Great points, after a re-read, I think your original suggestion is probably the right call here.

I think if DANGER_DISABLE_TRANSPILATION is truthy, or any env vars relating to ts-node (or maybe this TypeStrong/ts-node#846 (comment) ) are present we should not add those custom require handlers 👍🏻