RFC: Robust Lifecycle Scripts

[fritzy]

Proposal to implement more robust lifecycle scripts with reify context.

See issue npm/cli#3042

[wraithgar]

I think having discrete points in the reify process is better than trying to support the pre/post paradigm like this. It was an inadequate model and confuses people a lot (i.e. "pre" means "it runs before the x scripts" not "it runs before x")

I'd prefer not having pre/post be baked in like this. If we have a need for something to run at a time, we put an event there and name it accordingly.

[isaacs]

I agree. Also, I think it would be good to first make the list of the "event moments" we want to expose, and identify where each current scripts entry gets run. It would also give us a way to call out where the "no userland scripts allowed" phase of the process starts and ends (ie, everything between _loadTrees and _reifyPackages in lib/arborist/reify.js).

[ljharb]

imo "it runs before x" is the semantic every single person wants, and the other semantic is "silly sugar for &&".

[isaacs]

Suggested change

An `uninstall` script could assume that the directory will be removed or not used in the future, and put the package into a bad state if it was merely deduped, for example. By providing scripts with more context, like whether a new directory is being made or destroyed, which depenants caused the installation, etc, we could properly cover all use-cases and remove confusion that would lead to destroyed data, bugs, and frustration.

An `uninstall` script could assume that the directory will be removed or not used in the future, and put the package into a bad state if it was merely deduped, for example. By providing scripts with more context, like whether a new directory is being made or destroyed, which dependants caused the installation, etc, we could properly cover all use-cases and remove confusion that would lead to destroyed data, bugs, and frustration.

[isaacs]

We generally use the American spellings of things, so dependent (for both noun and adjective) rather than dependant. Sorry, total nitpick, I know. (Still reading, will provide more substantive feedback as full review.)

[wraithgar]

Something that was talked about in previous discussions about this was having some way to at least give people a fighting chance of making this an iterative change. Namely, that these could reference scripts. I don't think that's really the right solution but we should definitely keep that on our radar as we build examples. For instance making the lifecycle script entries call npm run xxx.

This does bring up the point of if these should be exclusive. i.e. if this object is populated at all, then the original lifecycle scripts are not ran.

[isaacs]

We generally use the American spellings of things, so dependent (for both noun and adjective) rather than dependant. Sorry, total nitpick, I know. (Still reading, will provide more substantive feedback as full review.)

[isaacs]

Suggested change

	See issue: #3042
	See: [#3042](https://github.com/npm/cli/issues/3042)

[isaacs]

I agree. Also, I think it would be good to first make the list of the "event moments" we want to expose, and identify where each current scripts entry gets run. It would also give us a way to call out where the "no userland scripts allowed" phase of the process starts and ends (ie, everything between _loadTrees and _reifyPackages in lib/arborist/reify.js).

[isaacs]

Suggested change

	"pre" "bin/life_install.sh --phase=pre ${dependant} ${depenant_path} ${reify-reason}"
	"pre" "bin/life_install.sh --phase=current ${dependant} ${reify-reason}"
	"pre" "bin/life_install.sh --phase=post ${dependant} ${reify-reason}"
	"pre": "bin/life_install.sh --phase=pre ${dependent} ${dependent_path} ${reify_reason}",
	"current": "bin/life_install.sh --phase=current ${dependent} ${reify_reason}",
	"post": "bin/life_install.sh --phase=post ${dependent} ${reify_reason}"

[isaacs]

If this is running during the reserved phase, then we're in trouble.

Before diffing, we don't know what will actually be done. Before unpacking, we don't have the scripts on disk to run. After unpacking, it's arguably not "pre"-install. But a userland script can modify the folder tree (or even the dependency graph, if it writes to package.json files), so it invalidates the work already done. We'd have to keep reloading the trees and recalculating the diff if any changes were made. If we don't do that, then we have security problems. Better to just keep the "pre" to "here's what we were asked to do, we are about to start getting ready to figure out how to do it", and the "post" is "here's what we did, and it's all the way done now".

So something like:

{
  "lifecycle": {
    "beforeTreeChange": "_loadTrees has not yet been called",
    "afterTreeChange": "_reifyPackages has completed",
    "beforeSave": "have not yet written to package.json and package-lock.json",
    "afterSave": "completely wrote lockfile and manifest"
  }
}

If beforeTreeChange and afterTreeChange exposed the various arguments that we currently pass into arborist.reify() (add, rm, update, etc.) then people could use that to decide whether this is an uninstall they care about. If afterTreeChange also had a way to get the diff somehow (though I'm not sure the best way to do this), then they could likewise pick the replacement for scripts.uninstall that most suits their need.

[fritzy]

It seems like we'd be exposing implementation pretty directly, which is subject to change. For example, a module-map based solution might not use trees at all. Is there a way we could be more future-proof and still be accurate about the lifecycle? Do package maintainers need all of that granularity?

[bnb]

Would this RFC be a reasonable one to also include enhancements to the lifecycle flags, or should that be a separate concern?