Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(node): Support executing npm package lifecycle scripts (preinstall/install/postinstall) #24487

Merged
merged 29 commits into from
Jul 10, 2024

Conversation

nathanwhit
Copy link
Member

@nathanwhit nathanwhit commented Jul 9, 2024

Adds support for running npm package lifecycle scripts, opted into via a new --allow-scripts flag.

With this PR, when running deno cache (or DENO_FUTURE=1 deno install) you can specify the --allow-scripts=pkg1,pkg2 flag to run lifecycle scripts attached to the given packages.

Note at the moment this only works when nodeModulesDir is true (using the local resolver).

When a package with un-run lifecycle scripts is encountered, we emit a warning suggesting things may not work and to try running lifecycle scripts. Additionally, if a package script implicitly requires node-gyp and it's not found on the system, we emit a warning.

Extra things in this PR:

  • Extracted out bits of task.rs into a separate module for reuse
  • Added a couple fields to process.config in order to support node-gyp (it relies on a few variables being there)
  • Drive by fix to downloading new npm packages to test registry

TODO:

  • validation for allow-scripts args (make sure it looks like an npm package)
  • make allow-scripts matching smarter
  • figure out what issues this closes

Review notes:

  • This adds a bunch of deps to our test registry due to using node-gyp, so it's pretty noisy

cli/args/flags.rs Outdated Show resolved Hide resolved
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

Great! LGTM!

cli/npm/managed/resolvers/local.rs Show resolved Hide resolved
cli/npm/managed/resolvers/local.rs Outdated Show resolved Hide resolved
cli/task_runner.rs Show resolved Hide resolved
@nathanwhit nathanwhit enabled auto-merge (squash) July 10, 2024 02:53
@nathanwhit nathanwhit merged commit ce7dc2b into denoland:main Jul 10, 2024
17 checks passed
@nathanwhit nathanwhit deleted the postinstall-scripts branch July 10, 2024 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants