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

[cspotcode]

Actually that may not be entirely true. There are caveats and limitations but I think the vm module might have something?

[what1s1ove]

Hello, guys! Do you have any news on this?

I think it's no secret that type="module" is becoming more prevalent. It would be nice to allow the .cjs extension at least.

Error:  Error: Could not find a 'dangerfile.js' or 'dangerfile.ts' in the current working directory

By the way, if the .cjs extension is used and the path to this file is specified in the CLI, such an error will occur.

npx danger ci --dangerfile ./dangerfile.cjs --failOnErrors --text-only

Unable to evaluate the Dangerfile
 
Hey there, it looks like you're trying to import the danger module. Turns out
that the code you write in a Dangerfile.js is actually a bit of a sneaky hack. 

When running Danger, the import or require for Danger is removed before the code
is evaluated. Instead all of the imports are added to the global runtime, so if
you are importing Danger to use one of it's functions - you should instead just
use the global object for the root DSL elements.

Another reason why I think it's necessary to prioritize ESM modules is that DangerJS is often used with other linters, such as commitlint. It's common to want to describe, for example, the project prefix in one file to use it across two configurations.

// project.config.js

const ProjectPrefix = {
  APPS: [`wd`],
  ENVIRONMENTS: [`production`, `development`],
}

// commitlint.config.js

import { ProjectPrefix } from './project.config.js'

const configuration = {
  extends: [`@commitlint/config-conventional`],
  parserPreset: {
    parserOpts: {
      issuePrefixes: ProjectPrefix.APPS.map((it) => `${it}-`),
    },
  },
  rules: {
    'references-empty': [2, `never`],
  },
}

(The DangerJS part is currently not working.)

// dangerfile.js

import { danger, fail } from 'danger'

import { ProjectPrefix } from './project.config.js'

const Config = {
  BRANCH_PATTERN: new RegExp(
    `^((${ProjectPrefix.APPS.join(`|`)})-[0-9]{1,6})-[a-zA-Z0-9-]+$|(${ProjectPrefix.ENVIRONMENTS.join(`|`)})$`,
  ),
}

const isTitleValid = Config.TITLE_PATTERN.test(danger.github.pr.title)

if (!isTitleValid) {
  fail(
    `Pull Request title should match the following pattern – ${Config.TITLE_PATTERN}.`,
  )
}

[pascal-hofmann]

It would be great to have ESM support for danger-js. We are using https://github.com/semantic-release/semantic-release inside a dangerfile.js. semantic-release switched to ESM with version 20 (current one is 22). So we are stuck with an outdated version now. :(

[fbartho]

I agree this is important and valuable. I think this is going to take collaboration from the community.

I did a light audit almost 2 years ago (above) and at least half of the problem issues I identified at the time are still open issues as of this writing (though they may have workarounds).

Knowing what I know about the community using DangerJS, I’m not sure I would support an ESM-only build of DangerJS. I think that means that we need to expose CJS and ESM hooks and build the library to support either style of consumers.

Somebody needs to make an attempt at getting DangerJS to treat the Dangerfiles as ESM, and that includes changing the module._compile detail to support either CJS or ESM. Once we have that proof of concept, then we can group together to see how much other stuff needs to change / we can figure out how to package everything together.

Ideally, we wouldn’t have to make ts-node a runtime dependency that our consumers need to have, as lots of people use TypeScript with other tools that strip/ignore the types, and so that would be a painful and large tweak for the consumers.

[sznowicki]

Wouldn't it solve most of the pains if for now dangerjs enables a way to import mjs from cjs?

From my experience it's totally doable this way via dynamic import (so, (await import('./dangerfile.mjs)).default.

This way danger can stay commonjs while surrounding tooling like semantic-release can be imported via ESM.