danger / danger-js

⚠️ Stop saying "you forgot to …" in code review
http://danger.systems/js/
MIT License
5.22k stars 363 forks source link

fix(node): #1180 - Enable ESM support #1455

Closed matthewh closed 1 month ago

matthewh commented 1 month ago

Greetings!

I submit this PR as a possible solution for #1180. I did what worked for me but given the number of variations of projects and typescript configurations that are in the wild there could be other issues that the danger development team is aware of.

General

If the dangerfile is an mts file, use typescript to transpile it to mjs and run the mjs variant instead.

tsconfig.json changes

Bump moduleResolution from node to node16.

With node

❯ node ../danger-js/distribution/commands/danger.js local --dangerfile dangerfile.mts --base main --staging
Unable to evaluate the Dangerfile
 Error [ERR_REQUIRE_ESM]: require() of ES Module /Volumes/Projects/multipart/danger-plugin-processenv/._dangerfile.mjs not supported.
Instead change the require of /Volumes/Projects/multipart/danger-plugin-processenv/._dangerfile.mjs to a dynamic import() which is available in all CommonJS modules.
    at /Volumes/Projects/multipart/danger-js/distribution/runner/runners/inline.js:171:114 {
  code: 'ERR_REQUIRE_ESM'
}

Danger: ⅹ Failing the build, there is 1 fail.
## Failures
Danger failed to run `dangerfile.mts`.
## Markdowns
## Error Error

require() of ES Module /Volumes/Projects/multipart/danger-plugin-processenv/._dangerfile.mjs not supported.
Instead change the require of /Volumes/Projects/multipart/danger-plugin-processenv/._dangerfile.mjs to a dynamic import() which is available in all CommonJS modules.
Error [ERR_REQUIRE_ESM]: require() of ES Module /Volumes/Projects/multipart/danger-plugin-processenv/._dangerfile.mjs not supported.
Instead change the require of /Volumes/Projects/multipart/danger-plugin-processenv/._dangerfile.mjs to a dynamic import() which is available in all CommonJS modules.
    at /Volumes/Projects/multipart/danger-js/distribution/runner/runners/inline.js:171:114

### Dangerfile
---------------------------------------------------------------------------------------------------------------------^

With node16

❯ node ../danger-js/distribution/commands/danger.js local --dangerfile dangerfile.mts --base main --staging

Danger: ✓ passed, found only messages.
## Messages
Changed Files in this PR: 
 - src/index.mts
orta commented 1 month ago

Wow, that seems to be a very elegant answer 👍🏻

SimenB commented 1 month ago

@orta this broke TS files.

https://github.com/jest-community/eslint-plugin-jest/pull/1615#issuecomment-2178122035

terrymun commented 1 month ago

Can confirm that this upgrade is breaking our DangerJS checks in our pipeline that is using TypeScript files.

matthewh commented 1 month ago

@SimenB I'll see if I can track down the eslint-plugin-jest issue. @terrymun Can you please point me to the pipeline you are referring to?

matthewh commented 1 month ago

Seems like it was an easy fix.

  if (safeConfig.compilerOptions.module) {
    if (!esm) {
      safeConfig.compilerOptions.module = "commonjs"
    } else {
      safeConfig.compilerOptions.module = "es6"
    }
  }

Will open another MR.

matthewh commented 1 month ago

Closing the loop here. Followup MR to adjust compiler options properly here https://github.com/danger/danger-js/pull/1456. Sorry I missed this on the first pass 😞

orta commented 1 month ago

Fix is deploying, thanks!