feat(cli) Add custom build hook to zapier commands

[cyberwitch]

This PR adds support for a custom build hook to zapier CLI commands.

By specifying a _zapier-build script in package.json, apps can ensure that script will run during zapier build, zapier push, etc. This is useful if someone wishes to write apps in TypeScript, or using Babel, since they can ensure they are never pushing a stale build that is out of sync with their sources.

If there is no script named _zapier-build, there will be no change in functionality.

[casshill]

Nice! I really like the idea of using the users defined npm scripts instead of prescribing behaviour. Random aside but how good would it be if the zapier push command checked for a zapier-push script and ran that?! If not it just runs the current zapier push flow. A lot of our apps we want to pass the skip npm install flag and it's a pain to have to run zapier push, get an error, then run npm run zapier-push

[eliangcs]

Nice enhancement! I have some minor suggestions in the comments. Can you also document this feature in README-source.md? Feel free to merge if you've addressed all my suggestions.

[cyberwitch]

Nice! I really like the idea of using the users defined npm scripts instead of prescribing behaviour. Random aside but how good would it be if the zapier push command checked for a zapier-push script and ran that?! If not it just runs the current zapier push flow. A lot of our apps we want to pass the skip npm install flag and it's a pain to have to run zapier push, get an error, then run npm run zapier-push

I'm not sure if that belongs in the public package we ship to partners, unless partners have a use case for overriding zapier push? It seems like it could lead to confusion on their part. I'm not sure I understand why we don't use zapier push ourselves, either.

[xavdid]

Like Chang-Hung said, we'll bump the version at a later date. I made one suggestion you should probably address, but this looks great!

My only other thought is whether this is actually a breaking change. zapier-build is a pretty common scripts key (I'm pretty sure it's a default in some of our examples). It's not uncommon to have a script like: "zapier-build": "zapier build". As written, I think we send the user into an infinite loop. Here's an example of that setup:

zapier-platform/example-apps/babel/package.json

Lines 11 to 13 in 56f1e53

	"zapier-build": "rm -rf lib && babel src --out-dir lib",
	"zapier-dev": "rm -rf lib && babel src --out-dir lib --watch",
	"zapier-push": "npm run zapier-build && zapier push",

So I think we need to:

• mark this as a breaking change
• probably also add a control to check if we're in a loop already (in case people don't read the update notes)
• maybe make the build key something that's less common or less likely to be in wide use (could be as simple as a leading _)

[cyberwitch]

@xavdid Hmm...that is a concern. I chose zapier-build because I hoped it wasn't already in use, compared to something like build that is definitely in use. I would much rather not cause any breaking changes with this...but it seems like the only way to do that is to guess a key that hopefully no one is using already.

I'm going to try checking out and searching across branches in zapier-platform-collection since that would probably be the safest way to confirm if a key is used, but not sure if it'll kill my mac doing the search 😅

[cyberwitch]

@xavdid You're right, zapier-build is used heavily by partners. z-build isn't though! I feel like we should be consistent with hook names if we might add others though, and try not to do something generic enough that another tool might conflict... I welcome any suggestions 😄

[xavdid]

We could be sure (enough) that a script key like _zapier-build-hook isn't in use, if we wanted to do something like that.

Also, per these docs, process.env.npm_lifecycle_event is set to the name of the current script. So to prevent recursion, we should check that we're not already in a hook before we kick another build off. Honestly npm might have protections around that, which would be great.

A last option is that we ask partners to put their hooks in a zapier-hooks key in the package.json (not in scripts at all). Then this isn't a breaking change, since no one has that key right now. It would be something like

scripts: { build: "tsc" },
"zapier-hooks": { "build": "build" }

which also lets us easily add more later.

Then the code is something like:

// build.js
if (_.get(pJson, ['zapier-hooks', 'build'])) {
  runCommand('npm', ['run', _.get(pJson, ['zapier-hooks', 'build'])])
}

We'd still want to check for recursion (if npm doesn't already) just in case someone puts zapier build in their build script, but at least it would be opt-in recursion ¯\_(ツ)_/¯

[xavdid]

Just tested, npm doesn't have any recursion protection:

But, each loop is slow enough that it's not hard for a person to quit. It'll make any sort of CI that builds pre-test really unhappy though.

    "recurse": "node -e 'console.log(`ok`)' && npm run recurse",

[cyberwitch]

I'm fine with a script called _zapier-build. I feel wary about adding arbitrary keys to package.json, and think it might be confusing to partners (plus, it's nice to be able to see exactly what zapier will be doing with npm run _zapier-build).

I'll try to add a recursion check, and update README-source.md as @eliangcs suggested.