Ambient declarations not working due to import/export

[carmanchris31]

The import and export statements in danger.d.ts are preventing the danger DSL from being available in the global scope:

This is a known issue in Typescript *.d.ts files (ambient declarations): https://github.com/microsoft/TypeScript/issues/28097

A potential solution per this comment is to wrap global declarations in a declare global { } block.

EDIT: This appears to do the trick based on a quick test. Here's a minimum set of changes: https://github.com/carmanchris31/danger-ambient-declarations-repro/commit/e897e351bb253505075606acc9fde51fc429776e?w=1

Where should one look in the code in order to attempt addressing this?

[orta]

I specifically choose to not have them available in the global scope, can you explain your reasoning for why you'd want that?

[carmanchris31]

Since they are, in fact, provided globally, similar to methods like describe/expect from jest.

In the documentation for plugins, it's explicitly stated that danger should not be imported (emphasis added):

Note, that you should not add a import { danger, et, cetera } from "danger" in your plugin. The dirty truth is that Danger completely ignores that at runtime, and just puts all of it’s exports inside the global context. You’ll get compiler errors, or confusing shadow variable issues. So assume that it’s there, and working fine.

The emphasized portions in particular aligns with the expectation that, in a TypeScript environment, I should have access to the ambient declarations. Since they aren't available, I'm left with TypeScript errors. If I import from danger, I get errors from the danger cli. It creates a painful dilemma.

[orta]

Makes sense - how we deal with this is:

in tests

declare const global: any;

describe("lighthouse()", () => {
  beforeEach(() => {
    global.warn = jest.fn();
    global.message = jest.fn();
    global.fail = jest.fn();
    global.markdown = jest.fn();
  });

  afterEach(() => {
    global.warn = undefined;
    global.message = undefined;
    global.fail = undefined;
    global.markdown = undefined;
  });

and in app-code

// Provides dev-time type structures for  `danger` - doesn't affect runtime.
import { DangerDSLType } from "../node_modules/danger/distribution/dsl/DangerDSL"
declare var danger: DangerDSLType
export declare function message(message: string): void
export declare function warn(message: string): void
export declare function fail(message: string): void
export declare function markdown(message: string): void

Which gets set up via the yeoman generator for plugins

[kilahm]

I may be digging up an old thread so feel free to lock and not respond.

@orta - could you explain your reasoning behind forcing consumers to declare the globally available functions provided by Danger?

Having an import from so deep in the package, with an explicit reference to node_modules smells of reaching into the internals, which could (and should) change without creating BC breaks.

I'd be happy to work on a PR to make the declaration of the functions and constants global, allowing this tsconfig.json to work:

{
  "compilerOptions": {
    "target": "es6",
    "module": "es2015",
    "moduleResolution": "Node",
    "types": ["danger", "node"]
  },
  "include": ["./*"]
}

[orta]

Yeah, I'm probably not changing my mind on this one. Only plugin authors have to do the hack, doesn't pollute global scope for users and it fits how people would expect it to work in a dangerfile.

[fbartho]

We could provide a file that plugin authors could directly reference that would hook up that global scope?

Danger-plugin-interface.d.ts (not sure if that would work?)

[carmanchris31]

Danger's very own DangerFile contains declaration boilerplate with explanation comments as a workaround for this issue: https://github.com/danger/danger-js/blob/master/dangerfile.ts#L1-L17

That's not what I'd expect to have to do every time I write a dangerfile that does something other than use existing plugins.

Perhaps an acceptable way of accomplishing this would be to add the action methods (warn, fail, message, schedule, etc.) to Danger's DSL. These can be namespaced if desired, e.g. danger.actions.fail()

The result of this would be that all consumers of danger could simply do this and have full access to the fully-typed Danger API:

import {DangerDSLType} from 'danger';

declare const danger: DangerDSLType;

Long-term, we would probably deprecate the use of the globally-injected actions in favor of the DSL-scoped versions.

[orta]

You should never need to include those helpers in user-land Dangerfile

That said, I think re-exporting the types from an index file is a cool idea 👍🏻

[airtonix]

You should never need to include those helpers in user-land Dangerfile

That said, I think re-exporting the types from an index file is a cool idea 👍🏻

wat?

without import { DangerDSLType} from 'danger';

with import { DangerDSLType} from 'danger';

it's really strange that you're injecting stuff into global space in the first place to be brutually honest.