Skip to content
This repository has been archived by the owner on Apr 24, 2024. It is now read-only.

After build hook #218

Merged
merged 2 commits into from
Nov 1, 2020
Merged

Conversation

jeremygooch
Copy link
Contributor

This PR makes it possible to run a command after the initial build and rebuilds.

@Nargonath Nargonath self-assigned this Nov 1, 2020
@Nargonath Nargonath added the enhancement New feature or request label Nov 1, 2020
Copy link
Owner

@Nargonath Nargonath left a comment

Choose a reason for hiding this comment

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

Thanks for your work @jeremygooch. Don't worry about the review comments, I'll fix them myself. I have some time to work on this package right now so I want thing to move ahead quickly. I just added them so you get some feedbacks on what your contribution. 👍

@@ -141,6 +143,7 @@ fs.emptyDir(paths.appBuild)
}

spinner.succeed();
runHook(afterRebuildHook);
Copy link
Owner

Choose a reason for hiding this comment

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

I think it'd be better to use ora to display a message before running the hooks so the user knows what is happening.

const exec = require('child_process').exec;

const child = exec(hook, (error, stdout, stderr) => {
console.log(`${stdout}`);
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think the template string is necessary here since stdout and stderr are already strings and you don't add anything to the string themselves.


const exec = require('child_process').exec;

const child = exec(hook, (error, stdout, stderr) => {
Copy link
Owner

Choose a reason for hiding this comment

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

We are not using the return from exec so no need to put in in variable IMO.

return;
}

const exec = require('child_process').exec;
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to put the require at the top of the file, just to be consistent with what has already been done.

@Nargonath Nargonath merged commit 7ecd36c into Nargonath:master Nov 1, 2020
Nargonath pushed a commit that referenced this pull request Nov 1, 2020
* Adding cli option after-build-hook.

* Specifying initial vs rebuild hooks.
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants