[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:

https://github.com/danger/danger-js/blob/c4fa1308d969af7acc1b2ae052c1a75fbe26a9c3/source/runner/runners/inline.ts#L67-L82

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?

https://github.com/danger/danger-js/blob/913db0e8ba751dacbc56609f4db2dabb4d8871aa/source/commands/utils/dangerRunToRunnerCLI.ts#L2-L57

[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 https://github.com/TypeStrong/ts-node/issues/846#issuecomment-631828160 ) are present we should not add those custom require handlers 👍🏻