Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

matthewh
Copy link
Contributor

@matthewh matthewh commented Jun 17, 2024

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

@matthewh matthewh force-pushed the danger-js-1180-esm_support branch from 3705481 to 7fb5f74 Compare June 17, 2024 21:49
@orta
Copy link
Member

orta commented Jun 19, 2024

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

@orta orta merged commit d99a404 into danger:main Jun 19, 2024
2 checks passed
@SimenB
Copy link
Contributor

SimenB commented Jun 19, 2024

@orta this broke TS files.

jest-community/eslint-plugin-jest#1615 (comment)

@terrymun
Copy link

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

@matthewh
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

matthewh commented Jun 19, 2024

Closing the loop here. Followup MR to adjust compiler options properly here #1456. Sorry I missed this on the first pass 😞

@matthewh matthewh deleted the danger-js-1180-esm_support branch June 19, 2024 16:29
@orta
Copy link
Member

orta commented Jun 20, 2024

Fix is deploying, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants