search

danger / danger-js

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

Error with importing Danger module in Dangerfile.js #1459

Open npatmedia opened 4 months ago

npatmedia commented 4 months ago

I'm encountering an error when trying to import the Danger module in my Dangerfile.js. Additionally, the provided Spectrum discussion thread link (https://spectrum.chat/?t=0a005b56-31ec-4919-9a28-ced623949d4d) is broken and returns a 404 error.

Error Message: throw "\nHey there, it looks like you're trying to import the danger module. Turns out\nthat the code you write in a Dangerfile.js is actually a bit of a sneaky hack. \n\nWhen running Danger, the import or require for Danger is removed before the code\nis evaluated. Instead all of the imports are added to the global runtime, so if\nyou are importing Danger to use one of it's functions - you should instead just\nuse the global object for the root DSL elements.\n\nThere is a spectrum thread for discussion here:\n - https://spectrum.chat/?t=0a005b56-31ec-4919-9a28-ced623949d4d\n"; ^ 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. There is a spectrum thread for discussion here:

Steps to Reproduce:

  1. Create a Dangerfile.js with the following content:

`import { danger, markdown, message, warn } from 'danger';

// Example usage of danger markdown('This is a markdown message');`

  1. Run the Danger.js script using Node.js v20.11.0. Expected Behavior: The Danger.js script should run without throwing an import error, and the appropriate actions should be executed as specified in the Dangerfile.js.

Actual Behavior: The script throws an error indicating that importing the Danger module is not allowed. Additionally, the provided link to the Spectrum discussion thread is broken and returns a 404 error.

Environment:

Node.js version: v20.11.0 Danger.js version: [Specify the version you are using] Additional Context: The error suggests that the import for Danger is removed before the code is evaluated, and all imports are added to the global runtime instead. However, there is no clear documentation on this behavior, and the broken Spectrum link makes it difficult to find further information or discussion on the topic.

Please provide guidance on how to properly use Danger.js with Node.js v20.11.0 and any updates on the broken Spectrum thread link.

orta commented 4 months ago

I assume then you don't have babel or typescript in your repo which would remove those imports I think, they are largely ceremonial and only exist to ensure a reasonable dev experience - they are actually globals, so you're OK to just write markdown or danger in the .js file 👍🏻

fbartho commented 4 months ago

I confirm @orta’s statements, in JS-alone, you can either remove the import statement, or convert it to a require

To elaborate: When I last tweaked the relevant code in DangerJS, import wasn’t a keyword for JS-only codebases. There’s probably work to be done around DangerJS usage with JS & ESM-imports in node20.

As you mentioned, we definitely should update things to no longer reference the Spectrum link, as Spectrum is defunct for us.

fbartho commented 4 months ago

This file is supposed to strip the import out: https://github.com/danger/danger-js/blob/92d2525fe338bff16ae7d42794d0a835e2d27473/source/runner/runners/utils/cleanDangerfile.ts

Some relevant call sites:

https://github.com/danger/danger-js/blob/92d2525fe338bff16ae7d42794d0a835e2d27473/source/runner/runners/inline.ts#L95

https://github.com/danger/danger-js/blob/92d2525fe338bff16ae7d42794d0a835e2d27473/source/platforms/GitHub.ts#L145

https://github.com/danger/danger-js/blob/92d2525fe338bff16ae7d42794d0a835e2d27473/source/platforms/github/customGitHubRequire.ts#L129

fbartho commented 4 months ago

One question: you’re not just trying to node Dangerfile.js or otherwise execute the dangerfile that way, right? How are you triggering danger?

(Danger has a CLI tool that you use to trigger it that ensures all the environmental conditions are right for your code to be executed correctly!)

npatmedia commented 2 months ago

sorry for late response. we use it with ts, SWC and webpack

it logs like /home/runner/work/webmobile-pwa/webmobile-pwa/node_modules/.pnpm/danger@12.3.3/node_modules/danger/distribution/danger.js:3 throw "\nHey there, it looks like you're trying to import the danger module. Turns out\nthat the code you write in a Dangerfile.js is actually a bit of a sneaky hack. \n\nWhen running Danger, the import or require for Danger is removed before the code\nis evaluated. Instead all of the imports are added to the global runtime, so if\nyou are importing Danger to use one of it's functions - you should instead just\nuse the global object for the root DSL elements.\n\nThere is a spectrum thread for discussion here:\n - https://spectrum.chat/?t=0a005b56-31ec-4919-9a28-ced623949d4d\n"; ^

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.

There is a spectrum thread for discussion here:

(Use node --trace-uncaught ... to show where the exception was thrown)

Node.js v20.11.0