refactor: remove support for reloading lockfile on update (#1701)

.github/workflows/ci.yaml

@@ -151,10 +151,7 @@ jobs:
                     # Don't run workspace smoke test under bzlmod
                     - bzlmod: 1
                       folder: e2e/workspace
-                    # Don't run update_pnpm_lock under bzlmod
-                    - bzlmod: 1
-                      folder: e2e/update_pnpm_lock
-                    # Don't run update_pnpm_lock under bzlmod
+                    # Don't run npm_translate_lock_auth under bzlmod
                     - bzlmod: 1
                       folder: e2e/npm_translate_lock_auth
                     # rules_docker is not compatible with Bazel 7
@@ -170,10 +167,6 @@ jobs:
                       folder: e2e/npm_link_package
                     - bzlmod: 1
                       folder: e2e/rules_foo
-                    - bazel-version:
-                          major: 7
-                      bzlmod: 1
-                      folder: e2e/update_pnpm_lock_with_import
                     # gyp_no_install_script is broken in an usual way on 6.5.0
                     # that is not worth investigating as we're dropping Bazel 6 support soon
                     - bazel-version:

docs/README.md

@@ -70,10 +70,10 @@ using the identical version of pnpm that Bazel is configured with:
 $ bazel run -- @pnpm//:pnpm --dir $PWD install --lockfile-only
 ```
 
-Instead of checking in a `pnpm-lock.yaml` file, you could use a `package-lock.json` or `yarn.lock`
-file with the `npm_package_lock`/`yarn_lock` attributes of `npm_translate_lock`.
-If you do, rules_js will run `pnpm import` to generate a `pnpm-lock.yaml` file on-the-fly.
-This is only recommended during migrations; see the notes about these attributes in the [migration guide](https://docs.aspect.build/guides/rules_js_migration).
+During migration from npm or yarn you can still use a `package-lock.json` or `yarn.lock` as the source of truth by
+setting the `npm_package_lock`/`yarn_lock` attributes of `npm_translate_lock`. When the `pnpm-lock.yaml` is out of date
+rules_js will run `pnpm import` to generate the `pnpm-lock.yaml` file.
+See the notes about these attributes in the [migration guide](https://docs.aspect.build/guides/rules_js_migration).
 
 Next, you'll typically use `npm_translate_lock` to translate the lock file to Starlark, which Bazel extensions understand.
 The `WORKSPACE` snippet you pasted above already contains this code.

e2e/update_pnpm_lock/test.sh

@@ -53,25 +53,33 @@ ASPECT_RULES_JS_DISABLE_UPDATE_PNPM_LOCK=
 # Have to make another change to package.json to invalidate the repository rule
 _sedi 's#"@types/node": "16"#"@types/node": "14"#' package.json
 
-# Trigger the update of the pnpm lockfile
-if ! bazel run "$BZLMOD_FLAG" @npm//:sync; then
-    echo "ERROR: expected 'bazel run $BZLMOD_FLAG @npm//:sync' to pass"
+# Trigger the update of the pnpm lockfile which should fail
+if bazel run "$BZLMOD_FLAG" @npm//:sync; then
+    echo "ERROR: expected 'update_pnpm_lock' to exit with non-zero exit code on update"
     exit 1
 fi
 
+# The lockfile should be updated
 diff="$(git diff pnpm-lock.yaml)"
 if [ -z "$diff" ]; then
     echo "ERROR: expected 'git diff pnpm-lock.yaml' to not be empty"
     exit 1
 fi
 
+# The action cache file should be updated
 action_cache_file=".aspect/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU="
 diff="$(git diff "$action_cache_file")"
 if [ -z "$diff" ]; then
     echo "ERROR: expected 'git diff $action_cache_file' to not be empty"
     exit 1
 fi
 
+# The lockfile has been updated and sync should now succeed
+if ! bazel run "$BZLMOD_FLAG" @npm//:sync; then
+    echo "ERROR: expected 'update_pnpm_lock' to succeed once the lockfile is up to date"
+    exit 1
+fi
+
 if ! bazel test "$BZLMOD_FLAG" //...; then
     echo "ERROR: expected 'bazel test $BZLMOD_FLAG //...' to pass"
     exit 1

e2e/update_pnpm_lock_with_import/test.sh

@@ -47,24 +47,34 @@ fi
 ASPECT_RULES_JS_FROZEN_PNPM_LOCK=
 
 print_step "It should update the lockfile after a running the invalide target with ASPECT_RULES_JS_FROZEN_PNPM_LOCK unset"
-if ! bazel run "$BZLMOD_FLAG" @npm//:sync; then
-    echo "ERROR: expected 'bazel run $BZLMOD_FLAG @npm//:sync' to pass"
+
+# Trigger the update of the pnpm lockfile which should fail
+if bazel run "$BZLMOD_FLAG" @npm//:sync; then
+    echo "ERROR: expected 'update_pnpm_lock' to exit with non-zero exit code on update"
     exit 1
 fi
 
+# The lockfile should be updated
 diff="$(git diff pnpm-lock.yaml)"
 if [ -z "$diff" ]; then
     echo "ERROR: expected 'git diff pnpm-lock.yaml' to not be empty"
     exit 1
 fi
 
+# The action cache file should be updated
 action_cache_file=".aspect/rules/external_repository_action_cache/npm_translate_lock_LTE4Nzc1MDcwNjU="
 diff="$(git diff "$action_cache_file")"
 if [ -z "$diff" ]; then
     echo "ERROR: expected 'git diff $action_cache_file' to not be empty"
     exit 1
 fi
 
+# The lockfile has been updated and sync should now succeed
+if ! bazel run "$BZLMOD_FLAG" @npm//:sync; then
+    echo "ERROR: expected 'update_pnpm_lock' to succeed once the lockfile is up to date"
+    exit 1
+fi
+
 print_step "It should pass a bazel test run"
 
 if ! bazel test "$BZLMOD_FLAG" //...; then

npm/private/npm_translate_lock.bzl

@@ -102,17 +102,13 @@ def _npm_translate_lock_impl(rctx):
         if state.action_cache_miss():
             _fail_if_frozen_pnpm_lock(rctx, state)
             if _update_pnpm_lock(rctx, state):
-                if rctx.attr.bzlmod:
-                    msg = """
+                msg = """
 
 INFO: {} file updated. Please run your build again.
 
 See https://github.com/aspect-build/rules_js/issues/1445
 """.format(state.label_store.relative_path("pnpm_lock"))
-                    fail(msg)
-                else:
-                    # If the pnpm lock file was changed then reload it before translation
-                    state.reload_lockfile()
+                fail(msg)
 
     helpers.verify_node_modules_ignored(rctx, state.importers(), state.root_package())
 

npm/private/npm_translate_lock_state.bzl

@@ -81,17 +81,6 @@ WARNING: `update_pnpm_lock` attribute in `npm_translate_lock(name = "{rctx_name}
         _copy_update_input_files(priv, rctx, label_store)
         _copy_unspecified_input_files(priv, rctx, label_store)
 
-def _reload_lockfile(priv, rctx, label_store):
-    _load_lockfile(priv, rctx, label_store)
-
-    if _should_update_pnpm_lock(priv):
-        _init_importer_labels(priv, label_store)
-
-    _init_root_package(priv, rctx, label_store)
-
-    if _should_update_pnpm_lock(priv):
-        _copy_unspecified_input_files(priv, rctx, label_store)
-
 ################################################################################
 def _validate_attrs(attr, is_windows):
     if is_windows and not attr.pnpm_lock:
@@ -626,7 +615,6 @@ def _new(rctx):
         set_input_hash = lambda label, value: _set_input_hash(priv, label, value),
         action_cache_miss = lambda: _action_cache_miss(priv, rctx, label_store),
         write_action_cache = lambda: _write_action_cache(priv, rctx, label_store),
-        reload_lockfile = lambda: _reload_lockfile(priv, rctx, label_store),
     )
 
 npm_translate_lock_state = struct(