Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: expose forge publish api #811

Merged
merged 6 commits into from
May 2, 2021

Conversation

basz
Copy link
Contributor

@basz basz commented Apr 29, 2021

an attempt to expose the publish feature from electron forge

see #637

Copy link
Member

@bendemboski bendemboski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

A couple of change requests:

  1. We should expose the publishTargets option as a command argument, e.g. ember electron:make --publish --publish-targets=@electron-forge/publisher-github,@electron-forge/publisher-s3 (or maybe ember electron:make --publish --publish-target=@electron-forge/publisher-github --publish-target=@electron-forge/publisher-s3 would be cleaner? can't remember what ember-cli's argument parsing makes easy and hard) because people might have different workflows for publishing the same project to different publisher targets.
  2. Let's add some tests to the make suite to cover these new options. I think it should be pretty straightforward to stub forge's publish API like we already do for the make/package API and then verify that we do/don't call it correctly according to the arguments, and that we forward 0, 1, or >1 publish targets to the API when calling it., according to whichever of the --publish-target or --publish-targets command option we land on.

@bendemboski
Copy link
Member

Also, thinking through and writing down a few other things for posterity, but I don't think anything here is actionable:

electron:make vs. electron:publish

We generally try to make the ember-electron command API mirror the electron-forge API as closely as possible, which means ideally we'd build an electron:publish command rather than adding a flag to the electron:make command. However, incremental workflow steps (e.g. reusing the output from previous steps) is also very important, and in this case electron-forge does not make that easy for us because the MakeResults that we'd have to pass to the publish API to use the in-filesystem output of a previous electron:make command are non-trivial to generate from the filesystem, in particular determining the list of artifacts, which would vary from maker to maker.

Ideally electron-forge would do something like create a JSON file as part of the make output containing the MakeResults so the publish command could be pointed at a make output directory and read the data back in from the JSON file. But as long as that isn't the case, it doesn't seem feasible for us to create an electron:publish command that can be pointed at the output of a previous electron:make command, so adding a --publish flag to the make command seems like the best way to achieve that right now.

We could potentially also implement an electron:publish command that just can't skip the making step (put could reuse the output of a previous electron:package command, like electron:make can). Then it could expose all the dry run functionality that forge supports in the publish API (see below), and so when users need that they could use it (but would have to sacrifice the ability to reuse make output).

Other publish options

I looked through the options that the electron-forge publish API accepts to see what we should support in the electron:publish command:

  • dir -- set internally to point to electron-app
  • dryRun/dryRunResume -- we could support these, but as mentioned above, it might be better to add them to an independent electron:publish command. Either way we can defer until someone actually wants it and we have known use case to target.
  • interactive -- could make sense for an independent publish command, but not worth supporting now
  • makeOptions -- not needed as long as --publish is a flag passed to electron:make
  • makeResults -- see above, not feasible to pass as CLI options given how the forge API works now
  • outDir -- only used for dry runs, so no value in supporting as long as we don't support the dry run arguments
  • publishTargets -- see above

@basz
Copy link
Contributor Author

basz commented Apr 30, 2021

Q: How do I run the make suite tests locally?

@bendemboski
Copy link
Member

You should be able to run yarn test-node-fast to run all the unit/integration tests, or yarn mocha <path to test file> to run tests in a single file.

@basz
Copy link
Contributor Author

basz commented May 1, 2021

think I copied, stole, pasted, and adapted some tests... please let me know what you think

@@ -157,4 +162,30 @@ describe('electron:make command', function () {
rimraf.sync('electron-app');
await expect(command.validateAndRun([])).to.be.rejected;
});

it('can publish', async function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start on the tests. It's currently only testing the command code and not the task code, though, because the task is being stubbed rather than the forge API method. So let's stop stubbing the task and instead use the forge API stub to verify results. Then we can also verify that the dir argument is being passed correctly, and that the make results are being conveyed properly.

So, this test would look like:

let makeResults = {};

// Change the make stub to return some fake results so we can ensure they
// they are passed to the publish API method
api.make.resetBehavior();
api.make.resolves(makeResults);

await expect(command.validateAndRun(['---publish'])).to.be.fulfilled;
expect(api.publish).to.be.calledOnce;
expect(api.publish.firstCall.args[0].dir).to.equal('electron-app');
expect(api.publish.firstCall.args[0].makeResults).to.equal(makeResults);
expect(api.publish.firstCall.args[0].publishTargets).to.be.undefined;

Then in the next two tests you can skip the make results stubbing since we've already verified that, and just assert that api.publish was called and that the publishTargets are correct.

@basz
Copy link
Contributor Author

basz commented May 2, 2021

of course, thank you for guiding me

Copy link
Member

@bendemboski bendemboski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, so glad to add publishing support -- thanks for the contribution!

@bendemboski bendemboski merged commit 14534a7 into adopted-ember-addons:master May 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants