hosting pre/postdeploy hooks now obey targeting

[driannaude]

Description

Fixes #1160 - pre/postdeploy hooks now obey targeting.

Scenarios Tested

Ran multiple targeted deploys

• Full deploy (firebase deploy)
• Only hosting (firebase deploy --only hosting)
• Only functions (firebase deploy --only functions)
• Hosting, 2 targets (firebase deploy --only hosting:x,hosting:y)
• Hosting, 3 targets (firebase deploy --only hosting:x,hosting:y,hosting:z)
• Functions, 2 targets (firebase deploy --only functions:x,functions:y)
• Functions, 3 targets (firebase deploy --only functions:x,functions,y,functions:z)

[coveralls]

Coverage increased (+0.04%) to 65.794% when pulling bb121f3 on driannaude:patch/fix-targeted-deploy-hooks into 04c8996 on firebase:master.

[bkendall]

Thanks for the PR!

@Memeriaj will have to take a look at this.

There also should be an entry added to CHANGELOG.md.

[Memeriaj]

Thank you for jumping in and fixing this.

From my own testing this will still run the pre- and post-deploy hooks when running firebase deploy --only functions but it will not run any of the hooks for individual functions deploys (firebase deploy --only functions:x. This is particularly annoying for functions because people will often write TypeScript functions and have a pre-deploy hook in order to compile their functions into the actual deployable JavaScript.

[driannaude]

Thanks guys, will take a look at the individual functions @Memeriaj, I think something is likely amiss in my testing!

[Memeriaj]

So I think that the easiest thing would be to short circuit functions specifically. Some like this on line 142 (although I haven't tried this myself quite yet):

// Functions can only have overall pre- and post-deploy hooks defined.
if (target === "functions" && onlyTargets.length > 0) {
  return config;
}

[driannaude]

I was looking into short-circuiting configs without targets this morning:

return targetConfigs.filter(function(config) {
  return !config.target || _.includes(onlyTargets, config.target);
});

It allows functions hooks to trigger if not targeted, and hosting can do targeted deploys, but my only concern is for hosting, where if target is not specified in firebase.json it will also run hooks for those configs.

However, a nice side effect is it allows users with targeted functions/hosting to specify items without targets as a "global" wrapper for hosting or functions e.g.

...
"functions": [{
  "predeploy": [
    "echo \"RUNNING PREDEPLOY HOOKS FOR ALL FUNCTIONS\"" // runs *once* for all functions
  ],
  "source": "functions"
},{
  "target": "api",
  "predeploy": [
    "echo \"RUNNING PREDEPLOY HOOKS API FUNCTIONS\"", // runs only for target:api function
    "npm --prefix ./functions run lint",
    "npm --prefix ./functions run build"
  ],
  "source": "functions"
}],

Changing the order/index of the global function also allows it to be run before or after everything else, or even between specific targets if required.

The only way I could "break" it was to specify a non-existing target (firebase deploy --only hosting:notarealtarget)

Which throws an unknown error that can probably be handled/made clearer by adding some validation to check if targets exist in the config before attempting to deploy them.

Additionally, when specifying a non-existing target and config options without target are present, it still triggers those lifecycle methods before throwing the unknown error.

@Memeriaj I'm not sure if this is changing intended functionality though, so I'm happy to just short-circuit functions as you suggested if you think that fits better with the overall flow of things 😄

Edit: spelling

[Memeriaj]

That seems pretty reasonable to me. Give me a little bit of time to reverse engineer any other crazy ways that people might break it (and also ask a couple other knowledgeable people in the office if they have weird usecases).

[Memeriaj]

Alright I think that your short-circuit is probably the best way of going about it. The only issues with using it that I could come up with are not usecases that we really want to support (I'll get into that further down). So switch over to that and update the chagelog so this will get captured in the release rollup and we should be good to go.

(Mainly for folks that happen upon this thread later for whatever reason)
The example firebase.json that you gave demonstrates the problem that can arise pretty well (mixing configs with targets with configs that do not have targets), however in practice this should not happen and the behavior is probably not intended. For Hosting you really should have a target on every one of your configs if you are using an array of them, the only time that you'd leave it off is if there is only the single config (in which case you don't even need to use the array, it can just be the config on its own). I'm fairly certain that Functions should never have an array of configs, mainly because each function should be defined in the code itself and there would just be the single config defined for all of them.

Which is basically just a long way of saying that all of the crazy cases that would break your proposed solution are things that people should not be doing.

[Memeriaj]

Is there anything else that you wanted to add to this? Otherwise I'll go ahead and squash and merge this into the master branch.

[bkendall]

If @Memeriaj is okay with this, so am I 🙂

[driannaude]

Hey, no I think we are good, this has been delightful! Cheers chaps.

[Memeriaj]

Perfect, this should go out in the next release (which will probably be next week at the latest, not completely sure since we usually wait for a couple of things group up).