[rush] `beforeInstall` hook invokes on a different set of conditions than `afterInstall` hook

[kevin-y-ang]

Summary

I was hoping that that if the beforeInstall hook runs, then there would be some guarantee that the afterInstall hook will run and vice versa. That turns out not to be the case in the following scenarios:

1. If installation is already up-to-date, then the beforeInstall hook will not invoke, but the afterInstall hook will.
2. If the installation fails, then the afterInstall hook will not invoke. This is unexpected because I was hoping to do some cleanup actions in the afterInstall hook.

It would be trivial for me to make this change in a PR but it would be a bit of a breaking change. Even so, I think it still might be worth considering because I think it's a common enough use case to use the 2 hooks in tandem, with the afterInstall hook being a cleanup function for the beforeInstall hook.

Source code file in question

Details

My use case is being able to measure granular subspace-by-subspace install duration metrics. This data would be much more useful for my team than the all-in-one durationInSeconds field provided by the flushTelemetry hook, which adds up the total duration of installation times for however many subspaces targeted by the rush install command.

Standard questions

Question	Answer
@microsoft/rush globally installed version?	5.149.0
rushVersion from rush.json?	5.149.0
useWorkspaces from rush.json?	No
Operating system?	Unix
Would you consider contributing a PR?	Yes
Node.js version (node -v)?	18.20.4

[dmichon-msft]

The reason for this discrepancy really comes down to that the hooks were added ad-hoc to support specific scenarios rather than having been holistically designed; e.g. the beforeInstall hook was introduced to support performing additional validation on package manager inputs, so if the package manager wouldn't be invoked, it didn't need to run. Currently the only place my team uses it is that we use it as a convenient trigger for pruning the local build cache under the assumption that if you did a new rush install or rush update (and the package manager actually had work to do) that your local cache entries are likely stale.

On the other hand, the afterInstall hook was added specifically to support the needs of rush-resolver-cache-plugin, which reads pnpm-lock.yaml and other artifacts of a successful installation and uses them to generate additional assets that only need to change if the installation changes.

If we were to standardize under what circumstances the hooks get invoked, then beforeInstall would need some property indicating whether or not an install will actually happen, and afterInstall would need to indicate if the install was successful or not, and rush-resolver-cache-plugin would need to be updated to not run if the install was unsuccessful.

[iclanton]

@kevin-y-ang - Would you be interested in putting together a change to make these behave the way you expect?

[kevin-y-ang]

Thank you for the context David.

@iclanton I'm interested in putting together a PR, but if you don't mind I'd like to wait a bit. My team will be depending on the rush plugin telemetry functionality and I'm hoping to let it marinate in a battle-tested environment to get a clearer idea of what's working well and what isn't so I don't hastily put together a change that doesn't make sense.