matthewh commented 1 month ago

Accidentally applied es6 when compilerOptions.module was not defined.

I recognize posting an error message to indicate a successful fix isn't great practice but I'm testing locally and this at least proves the code transpiled correctly and executed and failed once danger realized it's not inside a github PR.

"Success"

❯ pwd
/Volumes/Projects/multipart/eslint-plugin-jest
❯ danger local --base main --staging
...

Unable to evaluate the Dangerfile
 TypeError: Cannot read properties of  (reading 'pr')
    at Object.<anonymous> (dangerfile.ts:51:19)
    at Module._compile (node:internal/modules/cjs/loader:1376:14)
    at requireFromString (/Volumes/Projects/multipart/danger-js/node_modules/require-from-string/index.js:28:4)
    at /Volumes/Projects/multipart/danger-js/distribution/runner/runners/inline.js:183:68
    at step (/Volumes/Projects/multipart/danger-js/distribution/runner/runners/inline.js:56:23)
    at Object.next (/Volumes/Projects/multipart/danger-js/distribution/runner/runners/inline.js:37:53)
    at /Volumes/Projects/multipart/danger-js/distribution/runner/runners/inline.js:31:71
    at new Promise (<anonymous>)
    at __awaiter (/Volumes/Projects/multipart/danger-js/distribution/runner/runners/inline.js:27:12)
    at Object.runDangerfileEnvironment (/Volumes/Projects/multipart/danger-js/distribution/runner/runners/inline.js:123:132)

Addresses original error

Cannot use import statement outside a module
dangerfile.ts:46
import { parse } from 'path';
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at internalCompileFunction (node:internal/vm:76:18)
    at wrapSafe (node:internal/modules/cjs/loader:1283:20)
    at Module._compile (node:internal/modules/cjs/loader:1328:27)
    at requireFromString (/usr/src/danger/node_modules/require-from-string/index.js:28:4)
    at /usr/src/danger/dist/runner/runners/inline.js:183:68
    at step (/usr/src/danger/dist/runner/runners/inline.js:56:23)
    at Object.next (/usr/src/danger/dist/runner/runners/inline.js:37:53)
    at /usr/src/danger/dist/runner/runners/inline.js:31:71
    at new Promise (<anonymous>)
    at __awaiter (/usr/src/danger/dist/runner/runners/inline.js:27:12)

terrymun commented 1 month ago

Thanks for looking into that! I was about to create a MCVE on a dummy repo but you beat me to finding a fix :)

matthewh commented 1 month ago

@fbartho I'd love to hear your thoughts on how to test this. I noticed the __Dangerfile fixtures but I don't see anything that sets up a project to trigger this issue given the interplay with existing tsconfig settings. I'm confident I could work something up if desired. Maybe a unit test would be sufficient as well though I like the scaffolding around full project testing to more easily diagnose failures.

fbartho commented 1 month ago

@matthewh I was equally curious re the tests. This feels like it falls in the bucket of a pain to setup a test environment.

(I don't have any obvious/easy advice, hence my PR approval)

Once a year for the last few years I tripped over ESM issues for various projects, and tripped across edge cases with TypeScript, and I wasn't looking forward to needing to solve them here in DangerJS! At least this works with the setups we have tested, so that's incremental, good, progress in my opinion.

matthewh commented 1 month ago

@fbartho I think the variants distill to the following:

package.json - the value of module: boolean influences typescript.
<dangerfile> - there are 4 variants now mts, mjs, ts, and js.
tsconfig.json - danger only directly depends on compileOptions.modules though there are a huge number of settings that could influence the runner. Pick the most common and add new fixtures as bugs are reported. This brings up a question †

† why does danger need to use the current tsconfig at all? Because dangerfile imports live inside the current project. This seems messy when we consider that danger runs on top of a project and should never be a dependency. Ideally, I'd run npx danger and enforce a stricter set of guidelines for how a dangerfile should be structured. Of course this would likely mean danger would need to handle loading any dependencies at run time. Running danger would generate .danger/node_modules in the current directory adjacent to the dangerfile and keep all dependencies isolated. Overall this would reduce project interdependency on js projects though it sounds like a lift.

fbartho commented 1 month ago

No objections to your comments about the variants!

I don't think the way forwards is as obvious as you suggest @matthewh,

Ideally, I'd run npx danger and enforce a stricter set of guidelines for how a dangerfile should be structured.

For example I strongly disagree with using npx without locking danger in as a versioned package.json dependency (forgive me if I jumped to conclusions for what you were saying. I'm objecting to danger being an ambient global dependency of indeterminate or unspecified version).

Of course this would likely mean danger would need to handle loading any dependencies at run time. Running danger would generate .danger_modules in the current directory adjacent to the dangerfile and keep all dependencies isolated. Overall this would reduce project interdependency on js projects though it sounds like a lift.

I think you're right this would be a hell of a lift. I also don't think it's obvious that .danger_modules is easier for people to manage and maintain. I don't think that would necessarily simplify the full problem space. I could be convinced that that might simplify a subset of the problems, but I think it would make other maintenance/debugging/support/usage tasks that much more complicated.

matthewh commented 1 month ago

.danger_modules is a half baked idea 😁. In my mind you'd define any dependencies in your dangerfile and those would be loaded by danger. For example:

DEPENDENCIES = [
  "danger-plugin-...@<version>",
  "danger-plugin-...@<version>",
  "danger-plugin-...@<version>",
]

then when danger runs it would "npm install" these as needed.

It's an interesting problem space.

orta commented 1 month ago

Cool, discussion makes sense but lets get this shipped and folks can decide if there's more to do

danger / danger-js

fix(node): #1180 - Adjust tsconfig compiler options #1456

"Success"

Addresses original error