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 - Adjust tsconfig compiler options #1456

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

matthewh
Copy link
Contributor

@matthewh matthewh commented Jun 19, 2024

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)

Accidentally applied `es6` when compilerOptions.module was not defined.
@terrymun
Copy link

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

Copy link
Member

@fbartho fbartho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. This fix seems consistent with how the issue was introduced.

Not sure how easy this request is, but it feels like it’d be ideal to have a test case for this?

@matthewh
Copy link
Contributor Author

matthewh commented Jun 19, 2024

@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.

image

@fbartho
Copy link
Member

fbartho commented Jun 19, 2024

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

matthewh commented Jun 19, 2024

@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
Copy link
Member

fbartho commented Jun 19, 2024

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

.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
Copy link
Member

orta commented Jun 20, 2024

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

@orta orta merged commit f725706 into danger:main Jun 20, 2024
2 checks passed
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.

5 participants