Custom Publishing Strategies

[evoactivity]

So I was evaluating using this for releasing projects over at the @ember-tooling org. Most of what we need to publish are VSCode extensions though and right now npm is a baked in assumption of release plan.

One idea I came up with was having some way for a project to supply a custom publishing strategy by passing in a PublishStrategy class that would potentially look something like this.

import { PublishStrategy } from 'release-plan';

export default class VSCodeStrategy extends PublishStrategy {
  async doesVersionExist() {
    // do thing to determine version exists

    if (itExists) {
      return true;
    }
    
    return false;
  }

  async publish() {
    // do your publish steps

    if (isPublished) {
      return true;
    }
    
    return false;
  }
}

I think this would work by running a command similar to this

$ release-plan publish --strategy ./publish/vscode.js 

Popular strategies could potentially be merged back upstream and named so you can do things like this

$ release-plan publish --strategy vscode
$ release-plan publish --strategy npm
$ release-plan publish --strategy dockerhub

I'd like to take a stab at a proof of concept on this but creating this issue initially for feedback or other ideas.

Extra context https://github.com/orgs/ember-tooling/discussions/1

[NullVoxPopuli]

I think this is worth supporting (with some documented constraints aka "conventions")

• module must be ESM
• module must be JS
• module must have a default export
• module must extend from PublishStrategy (which implements errors by default for doesVersionExist and publish)

[ef4]

I'm OK with a plugin system. Reviewing the code, I can already see that there are places that would be clearer if we were using one internally (for example, for npm vs pnpm).

But if we make one I'm going to be picky and insist we do it properly, because nothing causes long-term pain like a half-thought-out plugin system. It's not the kind of thing you can just do as an MVP first and then make it better later. Once people are using it it's hard to change substantially.

• I don't want base class extension. That gives plugins too much access to implementation detail. It also creates an unnecessary need for a peerDependency, with all of the pain that peerDependencies bring. If instead the PublishStrategy is only an interface and not an implementation, a devDependency is enough to give you types, resulting in good feedback while developing a plugin.
• The existing publish behaviors need to become plugins.
  • makeTags
  • pushTags (maybe this and makeTags are really one plugin, I don't see a substantial reason they need to be separate)
  • npmPublish
  • pnpmPublish
  • gitHubRelease
• To reach feature parity with the existing code, plugins will need API for
  • an early precondition check that can stop the publish before any steps have actually started. For example, this is where the equivalent of

    release-plan/src/publish.ts

    Lines 315 to 318 in 95fb9c3

    	if (!process.env.GITHUB_AUTH) {
    	process.stderr.write(`\nYou need to set GITHUB_AUTH.`);
    	process.exit(-1);
    	}

    would happen in any plugin that talks to GitHub. The actual process.exit would not belong in the plugin. The plugin should probably just be given the opportunity to throw at the appropriate time, and an interface that lets us distinguish between an intentional user-readable message that we will display nicely vs an unexpected exception (in which case we'd fail with a stack trace).
    One might ask whether the plugin's class constructor can just be this phase, and I'd say probably not because it should support asynchrony (perhaps a plugin wants to actually test a credential at this step).
  • a way to report problems that happen too late to stop the publish. This is currently IssuerReporter

    release-plan/src/publish.ts

    Lines 324 to 326 in 95fb9c3

    	// from this point forward we don't stop if something goes wrong, we just keep
    	// track of whether anything went wrong so we can use the right exit code at
    	// the end.

    . Critically, it allows a plugin to report many problems, not just a single throw. Core should also catch around the plugins so one badly-behaved one that fails to use IssueReporter doesn't break everybody else. IssueReporter itself is written loosely because it wasn't expecting to be shared as public API. A version appropriate for plugins to use would not expose a writable hadIssues property (it should not be physically possible for a plugin to manipulate that). Probably the signature for the actual publish phase would be publish(reportFailure: (message: string) => void): Promise<void>, and no IssueReporter type is visible to the plugins.
• Plugins need to offer options. otp, publishBranch, and tag are all actually options to the npmPublish and/or pnpmPublish plugins, not the core as they are right now.
• All of this implies to me that we need a mandatory config file to list your plugins and their config. The corresponding CLI options would go away.
• We should design the typescript interfaces so it's possible to typecheck a plugin's options in the config file.
• The config file would do real ESM imports of each of the plugins, instead of just listing their names.
• Unlike the plugins, the config file should have a mandatory utility imported from release-plan that you use to construct your config. This is part of how we ensure good types, and it makes it easier to evolve the config over time with deprecations, etc.
• We should think about whether there are important interactions between the config file and the .release-plan.json. Is it OK to use a plan constructed under one config with a different config? At the moment, almost certainly yes because we're only talking about plugins that customize the publish step, and none of the other steps. But surely at some point someone is going to want to read a config file option in those other steps, at which point we probably want to have some safety check like putting a hash of the config file into the .release-plan.json so that you're forced to replan if you've changed the config.

If all this feels heavyweight, yeah, I don't disagree but that is the burden of evolvable extensibility. If people feel this isn't worth prioritizing I'm also OK with that decision.

[evoactivity]

Thanks for your detailed response @ef4!

I don't want base class extension. That gives plugins too much access to implementation detail. It also creates an unnecessary need for a peerDependency, with all of the pain that peerDependencies bring. If instead the PublishStrategy is only an interface and not an implementation, a devDependency is enough to give you types, resulting in good feedback while developing a plugin.

I don't disagree here, but for my curiosity, why would it need to be a peerDependency in the case of class extension but not an interface? Wouldn't a devDependancy work the same in either scenario? I'm also not clear what is meant by That gives plugins too much access to implementation detail., is the worry that if we change how PublishStrategy is implemented it would break existing plugins, or are there other negative effects you're concerned about?

The existing publish behaviors need to become plugins.

Agreed. I was planning on converting the current implementation to work through a plugin like structure before working on the vscode strategy, admittedly I was only considering the final publish steps, but making tag creation/publishing a plugin too makes sense.

an early precondition check that can stop the publish before any steps have actually started. For example, this is where the equivalent of ...code... would happen in any plugin that talks to GitHub. The actual process.exit would not belong in the plugin. The plugin should probably just be given the opportunity to throw at the appropriate time, and an interface that lets us distinguish between an intentional user-readable message that we will display nicely vs an unexpected exception (in which case we'd fail with a stack trace). One might ask whether the plugin's class constructor can just be this phase, and I'd say probably not because it should support asynchrony (perhaps a plugin wants to actually test a credential at this step).

Agreed. My initial idea just had doesVersionExist but I think I'd expand that to something more like verifyRequirements. I'd use something akin to IssueReporter for nice human errors, and then release plan can exit early if any plugin reports errors at this step. I also agree this needs to support asynchronous checks so the constructor wouldn't make sense.

a way to report problems that happen too late to stop the publish. This is currently IssuerReporter. Critically, it allows a plugin to report many problems, not just a single throw. Core should also catch around the plugins so one badly-behaved one that fails to use IssueReporter doesn't break everybody else. IssueReporter itself is written loosely because it wasn't expecting to be shared as public API. A version appropriate for plugins to use would not expose a writable hadIssues property (it should not be physically possible for a plugin to manipulate that). Probably the signature for the actual publish phase would be publish(reportFailure: (message: string) => void): Promise, and no IssueReporter type is visible to the plugins.

Agreed. I was planning on using IssueReporter for this anyway. I like the idea of only exposing a simple (message: string) => void type to the plugin.

Plugins need to offer options. otp, publishBranch, and tag are all actually options to the npmPublish and/or pnpmPublish plugins, not the core as they are right now.

Agreed. I think setting options would happen through the config file discussed later on. Though maybe a way for plugins to provide their own CLI arguments could be drafted. Do you have any thoughts yourself on how this might be achieved?

• All of this implies to me that we need a mandatory config file to list your plugins and their config. The corresponding CLI options would go away.
• We should design the typescript interfaces so it's possible to typecheck a plugin's options in the config file.
• The config file would do real ESM imports of each of the plugins, instead of just listing their names.
• Unlike the plugins, the config file should have a mandatory utility imported from release-plan that you use to construct your config. This is part of how we ensure good types, and it makes it easier to evolve the config over time with deprecations, etc.

Agreed. I'd also suggest

• The order of plugins in the config would be the execution order.
• Ability to configure the GH labels used for semver, possibly with multiple labels covering one of major, minor, patch etc.

We should think about whether there are important interactions between the config file and the .release-plan.json. Is it OK to use a plan constructed under one config with a different config? At the moment, almost certainly yes because we're only talking about plugins that customize the publish step, and none of the other steps. But surely at some point someone is going to want to read a config file option in those other steps, at which point we probably want to have some safety check like putting a hash of the config file into the .release-plan.json so that you're forced to replan if you've changed the config.

I like the idea of hashing the config, I don't see a reason not to have that functionality immediately. We could check the hash and warn the user with an option to continue anyway or to generate a new .release-plan.json from the new config.

As for prioritization, I wouldn't want to pull anyone away from more important work like embroider or core framework tasks to focus on something that is secondary and not needed for the projects currently using release-plan. So I'm totally willing to push ahead and work on this myself, I just might have questions or need some guidance as I work through things.

[mansona]

I like what @ef4 is saying here (great post btw, I wonder if it could be a blog on how to write a good plugin architecture 😂 ), but I have 2 issues that I want to be clear about:

All of this implies to me that we need a mandatory config file to list your plugins and their config. The corresponding CLI options would go away.

I would be very upset if this change meant that we forced a new config file on people that just want the "out of the box" implementation. Maybe we could take the vite approach and assume that if it's not there it uses the "default config" under the hood?

Secondly I understand that it's worth putting some thought into the .release-plan.json output at this stage because we're contemplating a plugin architecture that might be hard to change later, but I would really like to isolate the change to only the publish step for now. How would you feel about putting a "version": "v1" in the .release-plan.json output and punting any changes to a later date?

[evoactivity]

I would be very upset if this change meant that we forced a new config file on people that just want the "out of the box" implementation. Maybe we could take the vite approach and assume that if it's not there it uses the "default config" under the hood?

Agreed. I think an internal default config would be used if no .release-planrc.(cjs|mjs|js) could be found.

Secondly I understand that it's worth putting some thought into the .release-plan.json output at this stage because we're contemplating a plugin architecture that might be hard to change later, but I would really like to isolate the change to only the publish step for now. How would you feel about putting a "version": "v1" in the .release-plan.json output and punting any changes to a later date?

I'd be happy with that. I'd still probably work on the hashing functionality and bail out on any warning if a v1 plan is found, if people agree that that makes sense whilst only the publish step is being pluginised.

[ef4]

I'm not opposed to having a default config but I think in most projects that do that it becomes a gimmick and approximately 0% of real users actually use it without a config.

Also embroider uses --publish-branch=stable on the command line, which would be going away in favor of an option to the pnpm plugin. Could that become an env var instead, to avoid having a config file? Yeah, but I think that's harder to discover and learn by example than just having a config file, where mousing over the option in vscode would pop up the typescript-based documentation for the option.

[mansona]

I think it's totally fine for the stable branch to introduce a config file so that it can override that function, we don't need to mess with ENV variables to avoid config files.

I think in most projects that do that it becomes a gimmick and approximately 0% of real users actually use it without a config

I strongly disagree with this. I've probably installed half of the current uses of release-plan and only 2 of them need to use any command line arguments at all. That's why I would like it to work with a default config 👍