Add Toolkit.run

[jclem]

Why?

I've been finding myself using this pattern over and over:

main().catch(err => {
  tools.log.fatal(err)
  tools.exit.failure()
})

async function main() {
  // Action code
}

In order to reduce boilerplate when using async/await, I'm proposing tools.run:

Toolkit.run(async tools => {
  // Action code
}, { event: 'push' })

How?

This adds a simple static run function to the Toolkit class. It accepts an async function as an argument, and optionally some toolkit options. If that function throws an error, run will log the error and exit with a failure status.

---

• Tests have been added/updated (if necessary)
• Documentation has been updated (if necessary)

[JasonEtco]

Definitely agree with this 👌 I don't like the pattern of a main function at all, but I've also had to do something similar in every Action. I have done something else helpful - by having a separate function that is my "action code" that takes a Toolkit argument, I can pass a mocked Toolkit in tests. I need to document this for #50, but it vaguely looks like this:

module.exports = async tools => {
  if (!tools) tools = new Toolkit()
}

This lets me write an async function and do this in tests:

it('does something', async () => {
  const tools = new Toolkit()
  // Mock something on the Toolkit instance
  tools.getPackageJSON = jest.fn(() => ({ })
  await myAction(tools)
})

So with that in mind, what about an API like this:

class Toolkit {
  static public async run (fn: (tools: Toolkit) => Promise<void>, options?: ToolkitOptions) {
    const tools = new Toolkit(options)
    return fn(tools)
  }
}

And used like:

const { Toolkit } = require('actions-toolkit')
Toolkit.run(async tools => {
  return tools.github.repos.delete(tools.context.repo)
}, { event: 'issues' })

This seems like an easier way to export a function to test, but still run the action code. What do you think @jclem?

[jclem]

I think for the pattern of using a single action for many scenarios/actions that makes sense. Personally, I haven't actually done that yet, but I can see why one would.

I think this proposal is great. I can still use it the way I planned to if the options: ToolkitOptions argument is optional.

[JasonEtco]

I can still use it the way I planned to if the options: ToolkitOptions argument is optional.

Oh good point, yeah it should definitely still be optional (updated my above snippet). Also not sure if => Promise<void> is better than => Promise<unknown> or any 🤔

[jclem]

Also not sure if => Promise is better than => Promise or any 🤔

I think unknown is right—void would probably work, but it's not technically correct (it's not that the callback must return no value, it's that we don't care). unknown means "we can't use this without a type assertion", which is safer than any, where we can use the value if we wanted to.

I wonder if a generic makes more sense, but that seems overcomplicated, since we don't plan on using the value.

[JasonEtco]

unknown means "we can't use this without a type assertion"

Makes sense 👍 🚀

I wonder if a generic makes more sense, but that seems overcomplicated, since we don't plan on using the value.

Yeah agreed

[JasonEtco]

Thanks for doing this @jclem 🎉 🙏 I left a couple of small notes, I'll open up a separate PR for the changes to the exit methods but I don't think its a blocker. Let me know if I can make anything more clear!

[JasonEtco]

I hope you don't mind @jclem, I saw some activity on this PR so I finished it up. I updated the tests from the changes in #64, and tweaked one of the tests to be a little more "Jest-y." With that, this is good to 🚢 🎉! Thanks for your contribution ✨

[jclem]

@JasonEtco No problem at all, I appreciate the help. Got partway through the suggestions, and some things came up. Thanks for wrapping this up!