fix(cli): align _build_web_ui's npm closure with hermes update's (ui-tui + web + --include-workspace-root)
_update_node_dependencies() installs the unified closure, but update then calls _build_web_ui(), whose 'npm ci --workspace web' pass deleted node_modules and re-reified only the web closure — pruning root devDependencies and the ui-tui hoisted deps the previous step just installed, while exiting 0. Since the manifests digest was already recorded, later no-op updates skipped the repair. Reported by @andrexibiza in the #44772 final review (P1). Reproduced E2E: '--workspace web' alone removes typescript-eslint/@eslint/js from root node_modules; the unified closure restores them. Guards: ui-tui only named when its manifest exists (prebuilt checkouts), web-own-lockfile (#42973) and Termux (#38772) paths unchanged.
This commit is contained in:
+19
-1
@@ -5767,7 +5767,25 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool:
|
||||
# would pull in desktop on every web build. See #38772.
|
||||
# When web/ has its own package-lock.json, _workspace_root() returns
|
||||
# web_dir itself and --workspace would fail. See #42973.
|
||||
npm_workspace_args: tuple[str, ...] = () if npm_cwd == web_dir else ("--workspace", "web")
|
||||
#
|
||||
# When running from the workspace root, this must name the SAME closure
|
||||
# as `hermes update`'s _update_node_dependencies() (ui-tui + web +
|
||||
# --include-workspace-root): the helper prefers `npm ci`, which deletes
|
||||
# node_modules before reifying the requested tree, so a narrower closure
|
||||
# here silently prunes everything the update step just installed (root
|
||||
# devDependencies and the ui-tui workspace) while still exiting 0 —
|
||||
# and since the manifests digest was already recorded, later no-op
|
||||
# updates skip the repair. See #43564/#64354.
|
||||
npm_workspace_args: tuple[str, ...]
|
||||
if npm_cwd == web_dir:
|
||||
npm_workspace_args = ()
|
||||
else:
|
||||
npm_workspace_args = ("--workspace", "web", "--include-workspace-root")
|
||||
# Prebuilt/partial checkouts can lack the ui-tui workspace; naming a
|
||||
# missing workspace makes npm fail hard, so only include it when
|
||||
# present (same guard as _update_node_dependencies()).
|
||||
if (npm_cwd / "ui-tui" / "package.json").exists():
|
||||
npm_workspace_args = ("--workspace", "ui-tui", *npm_workspace_args)
|
||||
if _is_termux_startup_environment():
|
||||
npm_cwd, npm_workspace_args = _termux_workspace_install_context(web_dir)
|
||||
|
||||
|
||||
@@ -153,6 +153,59 @@ class TestBuildWebUISkipsWhenFresh:
|
||||
assert args[0] == ["/usr/bin/npm", "ci", "--include=dev", "--silent", "--prefer-offline"]
|
||||
assert kwargs["cwd"] == web_dir
|
||||
|
||||
def test_workspace_root_install_names_update_closure(self, tmp_path, monkeypatch):
|
||||
"""From the workspace root, _build_web_ui must install the SAME
|
||||
closure as `hermes update` (ui-tui + web + --include-workspace-root).
|
||||
|
||||
The install helper prefers `npm ci`, which deletes node_modules before
|
||||
reifying the requested tree — a narrower `--workspace web`-only pass
|
||||
right after the update step silently pruned root devDependencies and
|
||||
the ui-tui workspace while exiting 0. See #43564/#64354.
|
||||
"""
|
||||
web_dir, _ = _make_web_dir(tmp_path)
|
||||
# Root lockfile only => _workspace_root(web_dir) == tmp_path.
|
||||
(tmp_path / "package-lock.json").write_text("{}", encoding="utf-8")
|
||||
(tmp_path / "ui-tui").mkdir()
|
||||
(tmp_path / "ui-tui" / "package.json").write_text("{}", encoding="utf-8")
|
||||
monkeypatch.delenv("TERMUX_VERSION", raising=False)
|
||||
monkeypatch.setenv("PREFIX", "/usr")
|
||||
|
||||
install_cp = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="")
|
||||
build_cp = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="")
|
||||
with patch("hermes_cli.main.shutil.which", return_value="/usr/bin/npm"), \
|
||||
patch("hermes_cli.main.subprocess.run", return_value=install_cp) as mock_run, \
|
||||
patch("hermes_cli.main._run_with_idle_timeout", return_value=build_cp):
|
||||
result = _build_web_ui(web_dir)
|
||||
|
||||
assert result is True
|
||||
args, kwargs = mock_run.call_args
|
||||
cmd = args[0]
|
||||
assert "--include-workspace-root" in cmd
|
||||
assert cmd.count("--workspace") == 2
|
||||
assert "ui-tui" in cmd and "web" in cmd
|
||||
assert kwargs["cwd"] == tmp_path
|
||||
|
||||
def test_workspace_root_install_skips_missing_ui_tui(self, tmp_path, monkeypatch):
|
||||
"""A checkout without the ui-tui workspace must not name it — npm
|
||||
fails hard on a --workspace that doesn't exist."""
|
||||
web_dir, _ = _make_web_dir(tmp_path)
|
||||
(tmp_path / "package-lock.json").write_text("{}", encoding="utf-8")
|
||||
monkeypatch.delenv("TERMUX_VERSION", raising=False)
|
||||
monkeypatch.setenv("PREFIX", "/usr")
|
||||
|
||||
install_cp = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="")
|
||||
build_cp = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="")
|
||||
with patch("hermes_cli.main.shutil.which", return_value="/usr/bin/npm"), \
|
||||
patch("hermes_cli.main.subprocess.run", return_value=install_cp) as mock_run, \
|
||||
patch("hermes_cli.main._run_with_idle_timeout", return_value=build_cp):
|
||||
result = _build_web_ui(web_dir)
|
||||
|
||||
assert result is True
|
||||
cmd = mock_run.call_args[0][0]
|
||||
assert "ui-tui" not in cmd
|
||||
assert "--include-workspace-root" in cmd
|
||||
assert "web" in cmd
|
||||
|
||||
def test_web_build_uses_idle_timeout_helper(self, tmp_path):
|
||||
"""npm run build now goes through _run_with_idle_timeout (issue #33788).
|
||||
|
||||
|
||||
Reference in New Issue
Block a user