Skip to content

fix(nodejs): keep dev dependencies when installing at a workspace root - #947

Open
bnusunny wants to merge 1 commit into
aws:developfrom
bnusunny:fix/no-omit-dev-at-workspace-root
Open

bnusunny wants to merge 1 commit into
aws:developfrom
bnusunny:fix/no-omit-dev-at-workspace-root

Conversation

@bnusunny

Copy link
Copy Markdown
Contributor

Which issue(s) does this change fix?

Fixes the --cached half of aws/aws-sam-cli#6491.

Why is this change necessary?

--build-in-source on an npm workspaces monorepo deletes the root's own devDependencies from the customer's node_modules. The reporter of aws/aws-sam-cli#6491 loses typescript on every build and re-runs npm ci to get it back.

The per-function install runs in the function's directory, which for a workspace member is not where npm keeps the tree:

npm update --no-audit --no-save --omit=dev --no-package-lock --install-links

npm hoists one node_modules to the monorepo root, so that command reconciles the root's tree, and --omit=dev prunes the root's own dev dependencies while it is there. When the bundler is one of them the build does not merely damage the source tree, it fails:

Error: NodejsNpmEsbuildBuilder:EsbuildBundle - Esbuild Failed: Cannot find esbuild.

#941 fixed this for grouped builds, because prepare_shared_project installs once at the root and passes omit_dev=False deliberately. But grouping is skipped for --cached and --use-container builds, so those still install per function and still prune.

How does it address the issue?

get_install_action stops passing --omit=dev when an in-source install would reconcile an enclosing npm project root rather than the install directory itself.

That costs the artifacts nothing. The production closure is resolved at the link step (NodejsNpmLinkDependencyClosureAction, which consults dev_packages()), so dev dependencies are excluded there whether or not the install pruned them. --omit=dev was only ever a cheaper way to reach the same artifact, and in a monorepo it reaches it by damaging a tree this build does not own.

The "is npm's project root above this directory?" comparison existed inline already, for the artifacts link step. It is now one helper, enclosing_project_root, used by both callers.

Measured on SAM CLI 1.171.0 with this branch, npm 10.9.2, a workspaces monorepo declaring typescript, @types/node and esbuild only in the root's devDependencies:

build root devDependencies after build
--build-in-source --cached, flag on all three kept Succeeded (was: failed)
--build-in-source, flag on all three kept Succeeded (unchanged)
--build-in-source --cached, flag off all three removed Failed (unchanged)

And the artifact stays narrow: the built function's node_modules holds @repro/shared and lodash, its two declared dependencies, with typescript and @types/node absent.

Gated on experimentalNodejsMonorepo, so nothing changes until a caller opts in.

Checklist

  • Add input/output unit tests for the changed behaviour: 12 new tests covering the member install on both branches of get_install_action, the standalone project that must still prune, the flag-off and not-in-source cases that must not change, an explicit omit_dev=False that must not be turned back on, an unanswerable npm prefix, and enclosing_project_root directly including a path spelled differently from npm's answer.
  • Run make pr to verify: ruff check aws_lambda_builders clean, black --check clean, 917 unit tests pass.

Verified the tests catch the defect: with the fix reverted, test_a_workspace_member_install_keeps_dev_dependencies and test_the_lockfile_install_at_a_member_keeps_dev_dependencies_too both fail.

One fixture change outside the fix: tests/unit/workflows/nodejs_npm_esbuild/test_workflow.py gave popen a side_effect list of exactly one, which encodes the number of npm calls the workflow made that day — a build that asks npm one more question failed with StopIteration somewhere unrelated to the test's subject. It now returns the same fake for however many calls happen.

An in-source install run in a workspace member reconciles the monorepo root's
hoisted tree, so --omit=dev there deletes the ROOT's own devDependencies out of
the customer's node_modules - typescript, the test runner, the bundler - leaving
them to re-run `npm ci` after every build (aws/aws-sam-cli#6491). When the
bundler is one of the casualties the build fails outright with
`Esbuild Failed: Cannot find esbuild`.

Pruning a tree the build does not own buys the artifacts nothing: the production
closure is resolved at the link step, which leaves dev dependencies out by
itself. prepare_shared_project already passes omit_dev=False for this reason;
this extends the same decision to the per-function installs that still run,
--cached builds among them, where no shared install happens.

The path comparison that decides "is npm's project root above this directory"
is now one helper, enclosing_project_root, used by both the artifacts link step
and the new dev-dependency decision.

Gated on experimentalNodejsMonorepo, so nothing changes until a caller opts in.
@aws-sam-tooling-bot

Copy link
Copy Markdown

🧪 Hands-on test: No problems observed in 7 scenarios

Commit 8019469 · build LambdaToolingHandsOn-NoDocker:f4bbb3d2-17f0-4b43-aa7c-47a0ef46e11b. The plan's scenarios ran on an isolated build host; a problem is reported only if it reproduced at least twice. Passing scenarios show only that these checks behaved as expected, not that the change is correct.

Scenarios

Scenario Planned check Result Runner's note
S1 Main fix, lockfile branch: an in-source build of workspace member endpoints/fn with experimentalNodejsMonorepo (NodejsN… ✅ pass Workspace member endpoints/fn, lockfile kept, experimentalNodejsMonorepo on. Head: error null, is-number present in root node_modules before and after (dev_kept true), and the debug line 'NODEJS an i…
S2 The other branch of get_install_action: with the root package-lock.json removed, the member build takes the `npm update… ✅ pass Root package-lock.json removed, so the build takes the npm update path. Head ran 'npm update --no-audit --no-save --no-package-lock --install-links' without --omit=dev, logged the 'keeping dev depend…
S3 Rollout guarantee: without experimentalNodejsMonorepo, the member build must behave exactly as before (dev dependency p… ✅ pass Flag off. Head and base produced identical RESULTs: error null, is-number pruned from the root (dev_kept false), 'keeping dev dependencies' count 0. The helper exit code 1 comes only from the trailin…
S4 A standalone (non-workspace) project with the flag on is its own npm prefix, so the in-source install must still pass -… ✅ pass Standalone project with the flag on. Head still runs 'npm install ... --omit=dev', prunes is-number (dev_kept false) and keeps minimal-request-promise. artifacts/node_modules contains only minimal-re…
S5 Edge case for the realpath comparison in enclosing_project_root: a standalone project built through a symlinked path, w… ✅ pass Standalone project built through a symlinked source path with the flag on. Head called 'npm prefix', did not report an enclosing root ('keeping dev dependencies' count 0), and ran 'npm install' with …
S6 The headline failure from the PR description: the esbuild workflow building a workspace member in source, with esbuild … ✅ pass esbuild workflow on the workspace member, with esbuild only a root devDependency and none on PATH ('which esbuild' found nothing). Head: error null, dev_kept {is-number: true, esbuild: true}, 'keepin…
S7 The repo's own checks: the new and changed unit tests, then the full unit suite, since the esbuild test fixture change … ✅ pass Head unit tests: the nodejs_npm and nodejs_npm_esbuild workflow tests gave 87 passed. The new InSourceDevDependencies and EnclosingProjectRoot classes gave 11 passed. The full tests/unit suite gave 9…
Runner's notes in full
  • S1: Workspace member endpoints/fn, lockfile kept, experimentalNodejsMonorepo on. Head: error null, is-number present in root node_modules before and after (dev_kept true), and the debug line 'NODEJS an install in .../mono/endpoints/fn reconciles the tree at .../mono; keeping dev dependencies so it does not prune a tree it does not own' was logged. artifacts/node_modules = [@‍nodejs-workspaces-monorepo, fake-http-request, minimal-request-promise], with no is-number and no ms. Base: the same build ran 'npm install -q --no-audit --no-save --omit=dev --install-links', and is-number was gone from the root after the build (dev_kept false). Base artifacts were identical to head.
  • S2: Root package-lock.json removed, so the build takes the npm update path. Head ran 'npm update --no-audit --no-save --no-package-lock --install-links' without --omit=dev, logged the 'keeping dev dependencies' line, and kept is-number (dev_kept true). Artifacts contain @‍nodejs-workspaces-monorepo, fake-http-request and minimal-request-promise, with no is-number and no ms. Base ran the same command with --omit=dev and pruned is-number from the root.
  • S3: Flag off. Head and base produced identical RESULTs: error null, is-number pruned from the root (dev_kept false), 'keeping dev dependencies' count 0. The helper exit code 1 comes only from the trailing grep for an 'npm install' line finding nothing; the build itself exited 0. With the flag off, the artifacts have no node_modules on both trees. This is the pre-existing behaviour and not caused by the PR.
  • S4: Standalone project with the flag on. Head still runs 'npm install ... --omit=dev', prunes is-number (dev_kept false) and keeps minimal-request-promise. artifacts/node_modules contains only minimal-request-promise. 'keeping dev dependencies' count 0. Base result is identical.
  • S5: Standalone project built through a symlinked source path with the flag on. Head called 'npm prefix', did not report an enclosing root ('keeping dev dependencies' count 0), and ran 'npm install' with --omit=dev. is-number was pruned from the real fn/node_modules (dev_kept false), and the artifacts contain minimal-request-promise only.
  • S6: esbuild workflow on the workspace member, with esbuild only a root devDependency and none on PATH ('which esbuild' found nothing). Head: error null, dev_kept {is-number: true, esbuild: true}, 'keeping dev dependencies' logged, and artifacts contain a bundled, minified handler.js (1092 bytes). Base: the build failed with 'WorkflowFailedError: NodejsNpmEsbuildBuilder:EsbuildBundle - Esbuild Failed: Cannot find esbuild...', esbuild and is-number were pruned from the root, and the artifacts were empty. This reproduces the PR's headline bug on base and shows it fixed on head.
  • S7: Head unit tests: the nodejs_npm and nodejs_npm_esbuild workflow tests gave 87 passed. The new InSourceDevDependencies and EnclosingProjectRoot classes gave 11 passed. The full tests/unit suite gave 917 passed and 6 subtests passed, with exit 0.

🤖 Automated hands-on test — not a review approval · model claude-opus-5.5 · Kiro kiro-cli 2.33.1 · build LambdaToolingHandsOn-NoDocker:f4bbb3d2-17f0-4b43-aa7c-47a0ef46e11b · commit 8019469

History (1)
Commit Result When (UTC) Build
8019469 No problems observed 2026-10-30 01:30 LambdaToolingHandsOn-NoDocker:f4bbb3d2-17f0-4b43-aa7c-47a0ef46e11b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant