fix(lifecycle): skip verify for install hooks#8957
Conversation
0b3f89b to
832412d
Compare
832412d to
80aaf9e
Compare
|
We already have a lot of env variables set for scripts, can't we just use them? For instance, check for the existence of the |
|
@zkochan I have changed the code into checking for |
|
I see, but then I am not sure npm_lifecycle_event is needed. The previous approach was fine. |
| // are likely to also execute other `pnpm run`. | ||
| // We don't want this potentially expensive check to repeat. | ||
| // The solution is to use an env key to disable the check. | ||
| export const SKIP_ENV_KEY = 'pnpm_run_skip_deps_check' |
There was a problem hiding this comment.
Maybe we could avoid the additional env variable by setting this env variable instead:
npm_config_verify_deps_before_run=false
I think this will work unless someone explicitly sets the verify-deps-before-run setting via CLI options. However, in that case it is probably expected that we don't skip the check.
There was a problem hiding this comment.
Replacing it breaks some tests related to nested scripts. It probably means that it fails to skip check in some situations.
The code is in branch replace-custom-env-with-npm-config-env. The commit is 907b58a.
|
I think it's fine. The previous version of |
commit 49656eccb106c58ed7fa88d3aa7806bbb3410835
Author: Zoltan Kochan <z@kochan.io>
Date: Fri Jan 10 21:50:00 2025 +0100
test: fix
commit 4a2a961
Author: khai96_ <hvksmr1996@gmail.com>
Date: Sat Jan 11 03:04:09 2025 +0700
refactor: remove unnecessary restore step
commit 343459e
Author: khai96_ <hvksmr1996@gmail.com>
Date: Sat Jan 11 02:59:32 2025 +0700
test: fix
commit 907b58a
Author: khai96_ <hvksmr1996@gmail.com>
Date: Sat Jan 11 02:50:30 2025 +0700
refactor: replace env
| ] | ||
|
|
||
| export const shouldRunCheck = (env: Env, scriptName: string): boolean => !env[SKIP_ENV_KEY] && !SCRIPTS_TO_SKIP.includes(scriptName) | ||
| export const shouldRunCheck = (env: Env): boolean => !EVENTS_TO_SKIP.includes(env.npm_lifecycle_event) |
There was a problem hiding this comment.
Isn't it fine to do this
| export const shouldRunCheck = (env: Env): boolean => !EVENTS_TO_SKIP.includes(env.npm_lifecycle_event) | |
| export const shouldRunCheck = (env: Env): boolean => env.npm_lifecycle_event != null |
We won't ever verify deps in nested scripts anyway (because we set verify-deps-before-run to false) and npm_lifecycle_event isn't defined when you run the first pnpm run
that's probably because the tests are running inside |
Fixes #8954