[Feature Request] ESM Support

fbartho commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

TypeScript doesn’t have fully ergonomic support yet https://github.com/microsoft/TypeScript/issues/46452
ts-node has wonky / very awkward support for it: https://github.com/TypeStrong/ts-node/issues/1007
Jest has open bugs around ESM support https://github.com/facebook/jest/issues/9771
Node has issues: https://github.com/nodejs/node/pull/41552 https://github.com/nodejs/node/issues/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 commented 2 years ago

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 commented 2 years ago

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.

https://github.com/danger/danger-js/blob/a7355a33d1f414b1d2e3140ca0c0062e711823ea/source/runner/runners/inline.ts#L46-L74

cspotcode commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 [^1] and then import() it.

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

[^1]: necessary to ensure require and import relative paths work as expected; cannot emit to a temp directory

orta commented 2 years ago

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 commented 2 years ago

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

cspotcode commented 2 years ago

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

what1s1ove commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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.

danger / danger-js

[Feature Request] ESM Support #1180

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