Plugins

[deecewan]

I know that there is already a bunch of thought that has gone into this, but I think it'd be cool to start nailing down exactly what we'll allow plugins to do and how.

I've just had a couple of thoughts and wanted to get them down for posterity, and to get some feedback.

So, example Dangerfile:

import MyPlugin from 'my-plugin';

danger.register(new MyPlugin(/* config options for the plugin go here */));

// alternatively (see below for implementations)
import myPlugin from 'my-plugin';

danger.register(myPlugin);

MyPlugin:

export default class MyPlugin /* maybe `extends DangerPlugin` */ {
  constructor(options) {
    // handle whatever you need to
  }

  onWarn(danger /*not sold on passing danger into the hook. maybe into an init? */) {
    // send the warning to slack or something
  }

  onComplete(danger) {
    // notify slack of danger results or something
  }
}

or, myPlugin:

export default {
  onWarn(danger) {
    // do whatever. maybe a pretty console log
  }
}

Essentially we require an object. When something happens, we call the hook attached to that method. If it is more ephemeral, I think it could be just a plain old javascript object. Otherwise, if you wanted to maintain state more cleanly, use a class. I've used ES6 here, but there's nothing stopping people from writing it lower.

I think that if we provide a reference class to extend, then the super call in the constructor (or implicitly) could register a this.danger for access. Maybe otherwise as init function or similar to pass the root danger object in.

Is there an aspect to plugins I'm missing? What are your thoughts on this architecture?

[orta]

First off, I'd recommend taking a quick ten minute run through of the ruby "my first plugin" tutorial. Which gives a rough sense of how that works.It assumes no Ruby knowledge. Mainly as it took 3 attempts to eventually settle on a simple/extendable plugin system.

So far, and this may be a limitation of the ruby plugin infra, all plugins have been things that do some work and add their own warn/fail/messages, rather than acting upon warnings/messages etc

For example: http://danger.systems/plugins/prose.html - in Ruby we automatically expose a global for that plugin based on it's gem-name: example

# Look for prose issues
prose.lint_files

# Use the VS Code Spell-checker word ignore list
require 'json'
vscode_spellings = JSON.parse File.read(".vscode/spellchecker.json")

# Look for spelling issues
prose.ignored_words = vscode_spellings["ignoreWordsList"]
prose.check_spelling

I think for the case of JS, I had been wondering about how the simplicity of node_modules can make it easier.

import prose from "danger-prose"

prose.lint_files()

prose.ignored_words = ["ok", "sure"]
prose.check_spelling()

I moved the globals on to danger because I think we should pass that object to a plugin somehow. In which case, I think your situation may work, or we could check to see if a specific function is exported and allow DI-ing in that way. e.g.

const _danger: DangerDSL

export function setup(danger: DangerDSL) {
     _danger = danger
     // do any initialisation code
}

export function lint_files()  {
   const files = _danger.git.modified_files
   ...
  _danger.warn("This one failed: file.js")
}

or

const _danger: DangerDSL

export function setup(danger: DangerDSL) {
     _danger = danger
     // do any initialisation code
}

export default {
   lint_files()  {
     const files = _danger.git.modified_files
     ...
    _danger.warn("This one failed: file.js")
   }
}

It wouldn't be a problem then to also do class based exports too, for more complicated plugins.

We could definitely support a system of events too, perhaps allowing plugins to subscribe using the node EventHandler stuff?

[deecewan]

Oh wow. Yeah, i can't believe I didn't think of the case of distributing parts of the dangerfile.

I think that is a great call. I feel like maybe we could use attach or listen to do hooks/events. I only think hooks because it makes life a little easier from the start. And then we can also know when the plugin finishes.

In my head the plugins were to do things based on events, as is evident from my examples. I think doing both would be great, as there are obviously both situations, given we both had different ideas.

Keen for some more feedback. If I get on top of all my other stuff I might start a branch to play with this.

[orta]

OK, then other things that are important to me:

• We will want to be generating a page on the site for each plugins, so they need to be self documenting, a README isn't sufficient (as people don't put too much effort into those generally - for smaller projects)

  This is done using RDoc in Ruby, and JSDoc in JS. Source code documentation is basically the one thing I'll be pedantic on, to ensure everything keeps up to date

• We initially put time and effort into making it easy to automate a lot of the plugin setup process and to make a consistent API for plugins (so if you know how to use one, you probably will know how to use another. ) This pattern didn't really work well, and was dropped.

• In theory they can be source compatible directly with a Dangerfile, because the API (today) will be in global scope when running the project - though Explore changing the danger import handler to use the Jest runtime env #179 should/could/may change that. So I'm OK, with the "_danger" object that is passed in above. I think it will make it easy to write tests then too.

[orta]

WRT discoverability, we could have danger search though node_modules for and danger-xx then check whether their package.json confirms it's a danger plugin based on an arbitrary key-value.

If it is, we require the main file and check if the returned object has a setup method then run that first passing in the danger context. Then when a user includes the module, it's already set up? Or we can use the jest module control system to pre-run the setup function too ( #179 again )

[macklinu]

How does this plugin discussion relate to #18, if at all?

[orta]

This issue is the idea of "plugins" and the infrastructure to pull them off

I'm not too sure about whether #18 needs to exist, will write a note in there right now

[orta]

As of #227 - I think the dependencies checker is now good/complex enough to warrant turning into its own plugin.

[orta]

Figuring out plugins is not on my TODO before 1.0 - #225 BTW, so this is definitely open for someone to dig into

[macklinu]

I would love to help out on this. On my current client project, the number of codebases are growing, and being able to run the same checks across multiple codebases with a supported plugin system would be fantastic.

I see the value in a plugin being able to define a public API (similar to how Danger RB works):

import prose from "danger-prose"

prose.lint_files()

prose.ignored_words = ["ok", "sure"]
prose.check_spelling()

However, since a lot of Danger JS is global, most of the time I would trust a plugin to be registered with Danger, without me needing to explicitly call into a public API.

// example: a plugin called danger-plugin-yarn-lockfile-diff (for lack of a better name :D)
export default function dangerPluginYarnLockfileDiff() {
    const packageChanged = _.includes(danger.git.modified_files, 'package.json');
    const lockfileChanged = _.includes(danger.git.modified_files, 'yarn.lock');
    if (packageChanged && !lockfileChanged) {
        const message = 'Changes were made to package.json, but not to yarn.lock';
        const idea = 'Perhaps you need to run `yarn install`?';
        warn(`${message} - <i>${idea}</i>`);
    }
};

// my dangerfile
import { danger } from 'danger'
import dangerPluginYarnLockfileDiff from 'danger-plugin-yarn-lockfile-diff'

danger.register([
  dangerPluginYarnLockfileDiff,
])

// or maybe more explicitly
danger.executePlugins([
  dangerPluginYarnLockfileDiff,
])

I usually don't need to call into a public API for my current Danger "plugins" - they are just functions that can be executed as is.

I could see this register() or executePlugins() function taking an array of functions and executing them all.

If my plugin needs to take in a configuration option, I could export a function that returns a function:

// example: a plugin called danger-plugin-yarn-lockfile-diff (for lack of a better name :D)
export default function dangerPluginYarnLockfileDiff(config) {
  return function() {
    const {
      // defaults
      message = 'Changes were made to package.json, but not to yarn.lock'
      idea = 'Perhaps you need to run `yarn install`?'
    } = config || {}
    const packageChanged = _.includes(danger.git.modified_files, 'package.json');
    const lockfileChanged = _.includes(danger.git.modified_files, 'yarn.lock');
    if (packageChanged && !lockfileChanged) {
        warn(`${message} - <i>${idea}</i>`);
    }
  }
};

// my dangerfile
import { danger } from 'danger'
import dangerPluginYarnLockfileDiff from 'danger-plugin-yarn-lockfile-diff'

danger.executePlugins([
  dangerPluginYarnLockfileDiff({ message: 'Some message' }),
])

A benefit I see is simplicity - a plugin is basically just a function, which is easy to register and for Danger to execute.

A con is that perhaps this simple of an API could be confusing. If I import my Danger plugin function, am I supposed to invoke the function or just pass a reference to danger.executePlugins([])? In that sense, I see each plugin having its own public API being much clearer.

Thoughts on either of these?

[macklinu]

A couple other thoughts:

We can use package.json's keywords field for discoverability. Searching keywords:babel-plugin on npmjs.com allows me to see a bunch of babel plugins, for example. We could use "danger-plugin" as a keyword.

We could also create a Yeoman generator for Danger plugins once we come up with a system. It will be easy for devs to bootstrap a new Danger plugin. It will also help keep naming conventions consistent (if we want packages to be named danger-plugin-{pluginName}) as well as adding the "danger-plugin" to the generated package.json.

[orta]

Why not try for the simplest possible thing right now? You know that inside the context of "running in danger" there will be the danger const with all this set up.

So simplest thing is to make an NPM module, call it danger-plugin-yarn-lockfile-diff expose the function and really make it this simple:

export default function dangerPluginYarnLockfileDiff(config) {
  return function() {
    const {
      // defaults
      message = 'Changes were made to package.json, but not to yarn.lock'
      idea = 'Perhaps you need to run `yarn install`?'
    } = config || {}
    const packageChanged = _.includes(danger.git.modified_files, 'package.json');
    const lockfileChanged = _.includes(danger.git.modified_files, 'yarn.lock');
    if (packageChanged && !lockfileChanged) {
        warn(`${message} - <i>${idea}</i>`);
    }
  }
};

Then the Dangerfile can be:

import { danger } from 'danger'
import checkLockfile from 'danger-plugin-yarn-lockfile-diff'

checkLockfile()

Seems simpler overall, we can add the ability for an auto-config setup for plugins when it feels like it's a blocker for a plugin. As I'm not sure we'll need all the plugin infra overhead yet, if we prime people as "plugins alpha v0.1" I think we can start super small.

[deecewan]

So we just make the assumption that people will be happy to use the implicit global? I'm not a huge fan on the global pattern in JS, but I can see the uses for it here.

If that's the case, tho, there doesn't seem to be any extra work required. All of this is already possible currently, no?

I still think it'd be cool to provide hooks, etc. But obviously that can come much further down the track.

[orta]

For a plugins v1, sure, for plugins v2 I would also prefer to get rid of the globals magic for it too.

I took a few stabs at providing what I guess could be a way to do that in #180 (comment) - in theory it wouldn't be a breaking change to migrate, as danger would already exist in the runtime.

WRT hooks, I feel like those are a different thing form what I'm thinking of for step 1, I'd like to be able to run functions to share code from Dangerfiles not react to when events happen inside danger. I'd love to see that too, don't get me wrong, but I think the ability to migrate this out is a good first step

[macklinu]

Why not try for the simplest possible thing right now?

Sometimes it's so easy to jump 3 steps ahead instead of taking 1. Thanks for the perspective on this one, will start to extract some of my Dangerfile code into npm packages. 👍

[macklinu]

I created danger-plugin-no-test-shortcuts today. Do we want a plugins doc or section in our README for people to contribute links to their danger plugins for promoting reuse?

[deecewan]

Nice! I like it.

Maybe a separate page that contains a list of plugins? I can imagine a section in the readme getting rather long.

[orta]

There's a page set up on the site already for "how to create a plugin", which is here in source - so it'd be awesome to turn that into something real.

The is good because I can run through that tutorial for my lockfile stuff.

Once everything looks 👍 I'll make http://danger.systems/js index pick up all of the npm plugins in the same was that it does for Ruby

[orta]

OK, the plugin techniques from @macklinu definitely work, and I'm happy with it after building https://github.com/orta/danger-plugin-yarn

The generator makes great scaffolding: https://github.com/macklinu/generator-danger-plugin

So this is now unblocked WRT writing the page

[orta]

Mainly for my sake, as I wrote this, but can't use it, this'd make a nice plugin

// List available at https://github.com/artsy/emission/labels
const labelMatches = {
  Consignments: "Consignments",
  MSG: "Messaging",
  Messaging: "Messaging",
  Auctions: "Auctions",
}

// If you include any of the above keys in a commit in [], e.g. `[Consignments]`
// then the PR should be updated with the label providing context
const commitLabels = danger.git.commits
  .map(c => c.message)
  .filter(m => m.startsWith("[") && m.includes("]"))
  .map(m => m.match(/\[(.*)\]/)[1])

const labels = compact(uniq(commitLabels).map(l => labelMatches[l]))

const github = danger.github
schedule(() => github.api.issues.addLabels({ ...github.thisPR, labels }))

[orta]

https://gitlab.com/danger-systems/danger.systems/commit/70d802a297e8e597dc2cd2edf4e7d22011f919bd Adds plugins the the JS website index

[orta]

Plugins are 👌 now