[ENHANCEMENT] Enable refactoring dangerfile.ts into modules

[erheron]

Describe your issue We have pretty large dangerfile.ts with a lot of functions and rules, which could easily be refactored into, say, few functions, and the natural way is to put each function into its own module, for example in the danger folder.

For the sake of example let's say that our dangerfile.ts looks like this:

import {results, fail, message}  from 'danger';

function checkPR(...){
   const isBad = ...
   if(isBad){
      fail(...)
   }
}

function checkAllResults(){
   const {fails} = results;
   if(fails.length == 0){
      message("Great job!");
   }
}

checkPR();
checkAllResults();

And we want checkPR function to be inside its separate file danger/checkPR.ts, so our dangerfile would look like this:

import {results, message}  from 'danger';
import checkPR from './danger/checkPR';

function checkAllResults(){
   const {fails} = results;
   if(fails.length == 0){
      message("Great job!");
   }
}

checkPR();
checkAllResults();

Because we use Typescript, we would like the file danger/checkPR.ts to contain its own import {fail} from 'danger'. The problem is, it seems like the abovementioned construction would work only if danger/checkPR.ts contains NO import from danger at all. At the same time, I noticed that the import in dangerfile.ts gets removed automatically, and it just works fine both at the compile-time and run-time.

Suggestion/Question

I think it would be really nice to enable such refactoring, and if I'm not mistaking and the danger runtime is responsible for removing that import declaration at the top of dangerfile.ts, why not extend that rule to, say, all files in the danger folder (in root)? Another option I see is adding some annotation which would serve the same purpose

[erheron]

I've found this file: https://github.com/danger/danger-js/blob/3a0ff79b390019de6b622194feac69c98bb6b661/source/runner/runners/inline.ts#L84 And I'm wondering whether the proposed change could be implemented somewhere around this line 84 Also, it seems like there should be "then" instead of "they" ;)

[orta]

Yeah, I think making the transpiler func remove danger imports across all files it sees is a good idea 👍🏻

[erheron]

I will update with a little investigation:

Inside this function the processed file (I think it will be only dangerfile.ts) is cleaned and then compiled: https://github.com/danger/danger-js/blob/3a0ff79b390019de6b622194feac69c98bb6b661/source/runner/runners/inline.ts#L87-L91

The function which is responsible for "cleaning" is cleanDangerfile.js: https://github.com/danger/danger-js/blob/3a0ff79b390019de6b622194feac69c98bb6b661/source/runner/runners/utils/cleanDangerfile.ts#L1

The function which is responsible for compilation is this one: https://github.com/danger/danger-js/blob/3a0ff79b390019de6b622194feac69c98bb6b661/source/runner/runners/utils/transpiler.ts#L186

which in our case calls babelify which in turn calls transformSync

The proposed change could be implemented with a custom plugin for babel (which seems like an overkill to me), but I have a feeling that it can also be done earlier in this chain.

CC @orta

[erheron]

@orta wouldn't removing danger imports across all files require a lot of effort? Maybe we should narrow only to, say, danger/**/**.[js, ts]? That said, enforce a convention that if you want to refactor dangerfile, then put your functions inside danger folder in the root directory. This is just a random thought. Maybe it's not so hard as I think

[orta]

I don't think so. Danger owns the runtime, importing danger will always raise an exception if it gets through correctly. I also don't want the config and documentation weight from a describing and controlling that subset

[uncaught]

Be careful not to hard-code any folder names like /danger/, please!

What I've done to out-source functions is this - very cumbersome at the moment:

ci/gitlab/dangerfile.ts

import {danger, fail, schedule} from 'danger';
import validateMigrations from './validateMigrations';

validateMigrations({danger, fail, schedule});

ci/gitlab/validateMigrations.ts

import * as __dm from 'danger';

export default function validateMigrations({danger, fail, schedule}: {
  danger: typeof __dm.danger;
  fail: typeof __dm.fail;
  schedule: typeof __dm.schedule;
}): void {
  schedule(async () => {
    //...
    fail('...');
  });
}

[fbartho]

Personally, I include a file like this, and then all extra sourcefiles import from there instead of the danger module:

export {};
// DangerJS has a weird compile/runtime environment, but this is a stable detail!
// Briefly documented here:
// https://github.com/danger/danger-js/blob/master/docs/usage/extending-danger.html.md#writing-your-plugin

import { DangerDSLType } from "../../../node_modules/danger/distribution/dsl/DangerDSL";

// Provides dev-time type structures for  `danger` - doesn't affect runtime.
declare global {
  let danger: DangerDSLType;
  function warn(message: string, file?: string, line?: number): void;
  function fail(message: string, file?: string, line?: number): void;
  function markdown(message: string, file?: string, line?: number): void;
}