[Feature Request] ESM Support

[fbartho]

I got tripped up by a dependency that went "esm-only" today in their 7.x/latest version strip-ansi extra info from the maintainer.

Do we have a plan for ESM in DangerJS?

I'm not sure what this entails from DangerJS's perspective. I could write this off as an overly eager developer, but it smells like this is going to be an increasing question/stumbling block for people.

[orta]

I wasn't planning on moving danger, as you can't import "danger" so there's no difference, maybe if it meant it could run in deno it could be worth it.

Though I think maybe supporting dangerfile.mjs or dangerfile.mts maybe might make sense.

[fbartho]

Right, I’m much newer to understanding the practical consequences of ESM — I think your direct answers are useful, but slightly different from the crux of what I was getting at.

You said:

so there's no difference

I probably should have specified: this ESM-only package could not be imported or used in my Dangerfile. It caused an error. It’s fine for now because I was able to downgrade, but that’s not a long-term solution. — I selected this library by scanning my yarn.lock for likely keywords so this library was already a transitive dependency.

This means that now, I have a ticking time-bomb, where this dependency has consciously required ESM, and all my other dependencies that rely on it are A) prevented from upgrading it or B) will be forced to switch to a different provider of the feature, or C) it’s going to spread like a virus, forcing them to go ESM-only.

Now maybe I’m totally confused, and there should be a way for DangerJS to import this ESM library — is that the case?

[orta]

the dangerfile would be interpreted as either cjs or mjs based on the "type" in your package.json in node, or by the filetype (.cjs/.mjs) - its not on danger to do any different work if you're in an mjs or cjs package as that's still node responsibility, you just hit the timebomb in the same way you would if any of your (cjs) script files would try to import it

I could be wrong and you could be running with a package json as type: module, but I think you'd have added that context in the post

[BrunnerLivio]

Similar issue here - the latest packages of remark now require ESM.
Running danger locally result in this message:

Starting Danger PR on <PR_PATH>l#37
Unable to evaluate the Dangerfile
<PROJECT_PATH>/node_modules/remark/index.js:1
import {unified} from 'unified'
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at Object.compileFunction (node:vm:352:18)
    at wrapSafe (node:internal/modules/cjs/loader:1031:15)
    at Module._compile (node:internal/modules/cjs/loader:1065:27)
    at Object.customModuleHandler (/Users/brunnel6/workspace/roche-digital-platform/danger/node_modules/danger/distribution/runner/runners/inline.js:129:28)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/Users/brunnel6/workspace/roche-digital-platform/danger/node_modules/@roche-digital-platform/linter-markdown/dist/index.js:4:18)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)


Danger: ⅹ Failing the build, there is 1 fail.

I've added "type": module in my package.json, which results in the same issue. Seems like I am forced to downgrade as well :/

[fbartho]

I think what I interpreted from your reply @orta is that DangerJS is fine staying with CJS for its own internals for now, and that DangerJS doesn't care if your repo is ESM or CJS -- evaluating the Dangerfile should work either way.

At some point, we're going to need to upgrade one of our Dependencies because of security reasons, and that new version of the package might be ESM-only. I guess we can handle that when we have our first examples.

[fbartho]

Blockers we would have to resolve before we can make DangerJS be ESM:

• TypeScript doesn’t have fully ergonomic support yet Concerns with TypeScript 4.5's Node 12+ ESM Support microsoft/TypeScript#46452
• ts-node has wonky / very awkward support for it: ESM support: soliciting feedback TypeStrong/ts-node#1007
• Jest has open bugs around ESM support Support package exports in jest-resolve jestjs/jest#9771
• Node has issues: esm: handle extension-less "bin" files nodejs/node#41552 Invalidate cache when using import nodejs/node#49442 https://github.com/tc39/proposal-json-modules
• Node is planning to summarily delete __dirname which DangerJS uses in its unit tests. https://github.com/nodejs/node-eps/blob/master/002-es-modules.md#451-environment-variables

[fbartho]

Some libs that will need upgrading once we go to ESM:

• strip-ansi from 7.0.0 onwards
• node-fetch from 3.0.0 onwards
• execa from 6.0.0 onwards
• chalk from 5.0.0 onwards

(These libraries at older versions are all present in our yarn.lock)

[fbartho]

Actually, further investigation shows that DangerJS is using module._compile -- I think that means that DangerJS always interprets your Dangerfile in a CJS environment.

-- Note: the module keyword will be present for DangerJS's source-code since DangerJS is a CJS-compiled package. But module will not be present under ESM. I think that means that module._compile(…) will always execute in a CJS context? -- and that explains what @BrunnerLivio saw when he tried to upgrade his project to ESM.

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

Lines 46 to 74 in a7355a3

	/**
	* Executes a Dangerfile at a specific path, with a context.
	* The values inside a Danger context are applied as globals to the Dangerfiles runtime.
	*
	* @param {string} filename the file path for the dangerfile
	* @param {string} originalContents optional, the JS pre-compiled
	* @param {DangerContext} environment the results of createDangerfileRuntimeEnvironment
	* @param {any | undefined} injectedObjectToExport an optional object for passing into default exports
	* @param {func | undefined} moduleHandler an optional func for handling module resolution
	* @returns {DangerResults} the results of the run
	*/
	export const runDangerfileEnvironment = async (
	filenames: string[],
	originalContents: (string | undefined)[] | undefined,
	environment: DangerContext,
	injectedObjectToExport?: any,
	moduleHandler?: (module: any, filename: string) => string | Promise<any>
	): Promise<DangerResults> => {
	// We need to change the local runtime to support running JavaScript
	// and TypeScript through babel first. This is a simple implementation
	// and if we need more nuance, then we can look at other options
	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)
	}

[cspotcode]

I've subscribed to this issue and can explain any ts-node issues you may hit along the way. I'm not familiar with the way ts-node might be used here, if at all, but I see it mentioned above.

[fbartho]

@cspotcode thanks! ts-node is only used in 1 script and a comment, so I expect it will be minor compared to the other issues.

Currently those other problems are serious enough that I doubt DangerJS is going to be ESM-compatible for a while. This worries me on DangerJS's behalf, but also I don't think it's DangerJS' fault.

Maybe @orta has a clever idea on how to compile the Dangerfiles as ESM even if DangerJS is still CJS?

[cspotcode]

Typically I would recommend using ts-node to execute an .mts Dangerfile. But with node's current limitations around --loader, this would mean spawning a sub-process.

If we assume there are good reasons to avoid the above, then the next best thing I can think of is: write the compiled output to disk in the same directory as the dangerfile ¹ and then import() it.

const filename = `${dangerfilePath}${generateRandomFilenameSuffix()}.mjs`;
const code = compile(...);
fs.writeFileSync(filename, code);
const dangerfile = await import(filename);
fs.rmSync(filename);

Footnotes

1. necessary to ensure require and import relative paths work as expected; cannot emit to a temp directory ↩

[orta]

I'd assume there must be some new node APIs like module._compile which allow you to treat the file as though it was an .mjs file instead?

[cspotcode]

Unfortunately node only allows getting into the ESM loading path via `--loader`