danger / danger-js

āš ļø Stop saying "you forgot to ā€¦" in code review
http://danger.systems/js/
MIT License
5.28k stars 368 forks source link

"Unexpected token export" failure on dangerfile.ts #992

Open mokagio opened 4 years ago

mokagio commented 4 years ago

There's a chance this question is silly or obvious, due to my lack of knowledge of TypeScript and JavaScript. If that's the case, I apologize. šŸ™


I'm a bit surprised by the how Danger is behaving in regards to Dangerfiles with that do export default async () => { ... }.

If I run Danger against a Dangerfile that exposes its rules within that async function, like this one, I get this error:

Unexpected token export
path/to/dangerfile.ts
export default async () => {
^^^^^^
Click to see the full error log. ``` Unexpected token export peril-downloaded-Automattic/peril-settings/org/pr/label.ts:3 export default async () => { ^^^^^^ SyntaxError: Unexpected token export at Module._compile (internal/modules/cjs/loader.js:723:23) at Object.requireFromString [as default] (/home/runner/work/WordPress-iOS/WordPress-iOS/node_modules/require-from-string/index.js:28:4) at Object. (/home/runner/work/WordPress-iOS/WordPress-iOS/node_modules/danger/distribution/runner/runners/inline.js:144:63) at step (/home/runner/work/WordPress-iOS/WordPress-iOS/node_modules/danger/distribution/runner/runners/inline.js:32:23) at Object.next (/home/runner/work/WordPress-iOS/WordPress-iOS/node_modules/danger/distribution/runner/runners/inline.js:13:53) at /home/runner/work/WordPress-iOS/WordPress-iOS/node_modules/danger/distribution/runner/runners/inline.js:7:71 at new Promise () at __awaiter (/home/runner/work/WordPress-iOS/WordPress-iOS/node_modules/danger/distribution/runner/runners/inline.js:3:12) at exports.runDangerfileEnvironment (/home/runner/work/WordPress-iOS/WordPress-iOS/node_modules/danger/distribution/runner/runners/inline.js:105:136) at Object. (/home/runner/work/WordPress-iOS/WordPress-iOS/node_modules/danger/distribution/platforms/GitHub.js:169:38) ```

I can fix the issue if I run everything sync, like here.

To be clear this is what I mean by run everything sync

  // dangerfile.ts
  import {warn, danger} from "danger";

- export default async () => {
-   warn("A warning")
- };
+ warn("A warning")

I wouldn't find this too surprising if it wasn't for the fact that the same Dangerfile with export default async () => { ... } works when run using the Danger GitHub Action. šŸ¤” .

Real life examples:

Looking at the examples in the "About the Dangerfile" guide, the only Dangerfile I could find using that syntax was danger-js/dangerfile.ts but I can't find an example of it behaves when run.

At this point, my assumption is that the async code works for Peril but not for Danger, because of differences in the internals. It also works when Danger is used as a GitHub Action because of how the GitHub Action is implemented. Otherwise, one should avoid export default async () => { ... } in Dangerfiles. Is this correct? If it isn't (which is what I hope) what is it that I'm missing? How can I get it to work async?

mokagio commented 4 years ago

Solved!

I run DEBUG=* danger pr and scrolling through the log I noticed a "Does not have TypeScript set up" message.

I found the message in the transpiler source https://github.com/danger/danger-js/blob/92c2040c15947b6eb24182b46ff026a480936229/source/runner/runners/utils/transpiler.ts#L35

Now it all makes sense! I was wondering how Danger was able to read both JS and TS files. I does so by relying on the user's tools šŸ’” .

Once I yarn add typescript -D to my project which only uses Node for Danger JS, everything worked šŸ˜„

mokagio commented 4 years ago

I'm going to close this issue, but I wonder if Danger could do something better to let users know that they're running a .ts file without having TypeScript set up. cc @danger-systems

orta commented 4 years ago

Yeah, leaving this open for someone to make an error message around this šŸ‘

I'm fine with it fully bailing and raising an exception in this case