Conversation
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
e0d4fe9 to
ce5004a
Compare
ce5004a to
9c5f169
Compare
9c5f169 to
1ba2c01
Compare
There was a problem hiding this comment.
Code Review Results
Reviewed: 587257c..1ba2c01
Files: 15
Comments: 2
Comments on lines outside the diff:
[aws_lambda_builders/workflows/nodejs_npm/workflow.py:176] [GENERAL] Re-raising the artifact concern from the earlier review round — it was confirmed rather than dismissed, and the resolution was to move the test to the esbuild suite rather than address the behaviour.
Your own measurement of the workspaces-monorepo fixture through the non-esbuild NodejsNpmWorkflow produced artifacts/ = ['included.js', 'package.json'] with no node_modules, and require.resolve('minimal-request-promise') failing from the artifacts directory. The cause is that npm hoists to the monorepo root, so packages/fn/node_modules never exists, and LinkSinglePathAction then returns at LOG.debug level:
if not source_path.exists():
# Source path doesn't exist, nothing to symlink
LOG.debug("Source path %s does not exist, skipping generating symlink", source_path)
returnThe esbuild test added here passes because esbuild bundles the dependency into included.js, which masks the gap. For the plain npm workflow the result is a deployable package with no dependencies and no warning to the user — a failure that only surfaces at Lambda invocation time with MODULE_NOT_FOUND.
This is pre-existing rather than introduced here, but this PR makes workspaces monorepos a deliberately supported build-in-source path, so it is now in scope. At minimum, the missing node_modules in this configuration should be surfaced (a LOG.warning at the workflow level when building in source and the expected source node_modules is absent) rather than silently skipped. If you consider it out of scope, please say so explicitly and link a follow-up issue so it is not lost.
1ba2c01 to
40ed159
Compare
40ed159 to
34e1339
Compare
There was a problem hiding this comment.
Code Review Results
Reviewed: 587257c..34e1339
Files: 15
Comments: 1
Comments on lines outside the diff:
[aws_lambda_builders/workflows/nodejs_npm/workflow.py:176] [GENERAL] Re-raising the artifact gap from the previous two rounds. It was confirmed by your own measurement rather than declared acceptable, and the resolution — moving the monorepo fixture into the esbuild suite — routes around the behaviour instead of recording it.
For a workspaces monorepo built in source, npm resolves its project root to the monorepo root (the same resolution get_lockfile_path now relies on), so it hoists dependencies into /node_modules and never creates packages/fn/node_modules. _actions_for_linking_source_dependencies_to_artifacts then links source_dir/node_modules, and LinkSinglePathAction.execute returns silently when the source path is absent:
if not source_path.exists():
# Source path doesn't exist, nothing to symlink
LOG.debug("Source path %s does not exist, skipping generating symlink", source_path)
returnThrough the plain nodejs_npm workflow that yields artifacts/ without node_modules, matching what you measured (['included.js', 'package.json'], require.resolve('minimal-request-promise') failing). The new test only exercises the esbuild workflow, where the bundler inlines the dependency and the missing link cannot be observed — the require(bundle) assertion passes for a bundled import regardless.
I accept this hoisting predates the PR (npm update --no-package-lock --install-links resolves the same project root). The concern is that the PR adds the first workspaces-monorepo fixture and asserts it works, while the one workflow where it silently produces an incomplete artifact stays untested. Either add a plain-nodejs_npm test over workspaces-monorepo asserting current artifact contents (an xfail-style record is fine), or note the limitation next to the linking property so the gap is discoverable rather than implied to be covered.
34e1339 to
41f2ff4
Compare
|
Recorded in the code in 41f2ff4, which is the second of the two options you named.
# Known gap (aws/aws-lambda-builders#933): in an npm workspaces monorepo npm hoists to the
# monorepo root, so source_dir/node_modules is never created and LinkSinglePathAction skips
# silently, leaving the artifacts without dependencies. Pre-dates the lockfile lookup below and
# applies to this workflow only - the esbuild workflow bundles the dependency into the output.#933 (filed before this round) carries the measurement, the mechanism, the code pointer and a repro. The PR description says the same thing in prose, so the limitation is discoverable from the code, the issue and the description rather than implied away by the new workspaces coverage. I did not take the first option. An xfail-style test over Your reading of the esbuild assertion is right and worth stating plainly: Gates on 41f2ff4: 958 unit+functional passed at 94.67% coverage, |
41f2ff4 to
08597d0
Compare
08597d0 to
bda8848
Compare
d42b637 to
73e9e1c
Compare
73e9e1c to
27db041
Compare
27db041 to
47f952e
Compare
47f952e to
910de6d
Compare
910de6d to
74d8888
Compare
74d8888 to
2376a72
Compare
Building nodejs dependencies in source ignored the project's lockfile: the install step ran `npm update` with `--no-package-lock`, so every build re-resolved the ranges in `package.json` and could upgrade dependencies in the developer's own source tree. On a workspaces project whose root `package-lock.json` pins `lodash` 4.17.20 (manifest `^4.17.20`), one `sam build --build-in-source` left 4.18.1 installed. Behind an opt-in flag while it rolls out. `is_nodejs_monorepo_support_enabled` reads `experimentalNodejsMonorepo` from the `experimental_flags` the workflow is constructed with - sam-cli's `ExperimentalFlag.NodejsMonorepo`, whose config key crosses the boundary as that string, the same way `experimentalBuildPerformance` already does for python_pip. Without the flag every in-source build keeps `npm update --no-package-lock`, and npm is not even asked where the lockfile is. With it, and a lockfile npm will use: | project | before | with the flag | |---|---|---| | has a lockfile | `npm update --no-audit --no-save --omit=dev --no-package-lock --install-links` | `npm install -q --no-audit --no-save --omit=dev --install-links` | | has no lockfile | `npm update ...` | unchanged | Which directory holds the lockfile npm will use is npm's own decision, so `NodejsNpmWorkflow.get_lockfile_path()` asks it: `npm prefix`, run in the install directory, reports the monorepo root for a workspace package and the package itself for any other nested package, whose ancestors' lockfiles npm ignores. Identical on npm 8.19.4, 9.9.4, 10.9.9 and 11.19.0. `NodejsNpmInstallAction` takes back an `install_links` argument. `npm ci` stays gated on a lockfile in the source directory itself - run from a workspace package it empties the workspace root's devDependencies on every npm version. Lockfile-less projects keep `npm update` and its pruning of dependencies removed from the manifest (aws#579); the lockfile-respecting install prunes those as well, so projects with a lockfile lose nothing.
2376a72 to
90c3822
Compare
| # with no lockfile anywhere there are no locked versions to install, and `npm update` additionally | ||
| # prunes dependencies that were removed from the manifest since the previous build | ||
| can_use_links_mock.return_value = True | ||
| get_lockfile_path_mock.return_value = None |
There was a problem hiding this comment.
[GENERAL] This test does not exercise the path its name and comment describe. The workflow is constructed without experimental_flags, so self.experimental_flags == [] and the gate short-circuits before get_lockfile_path is ever consulted:
if is_nodejs_monorepo_support_enabled(experimental_flags) and NodejsNpmWorkflow.get_lockfile_path(...):The get_lockfile_path_mock.return_value = None is therefore inert — the assertion would still pass if the mock returned a lockfile path. What is actually being pinned is the flag-off path, which test_without_the_flag_a_lockfile_is_ignored_and_the_update_runs already covers (and covers more precisely, with run.assert_not_called()).
Add experimental_flags=["experimentalNodejsMonorepo"] to the constructor so the test fails if the flag-on/no-lockfile combination ever stops selecting NodejsNpmUpdateAction.
| @patch("aws_lambda_builders.workflows.nodejs_npm.workflow.NodejsNpmWorkflow.can_use_install_links") | ||
| def test_build_in_source_without_lockfile(self, install_links_mock, get_lockfile_path_mock): | ||
| install_links_mock.return_value = True | ||
| get_lockfile_path_mock.return_value = None |
There was a problem hiding this comment.
[GENERAL] Same inert-mock problem as the nodejs_npm counterpart: test_build_in_source_without_lockfile patches get_lockfile_path to return None but omits experimental_flags, so the monorepo gate short-circuits and the patched return value is never read. The test passes for the wrong reason and would keep passing if the flag-on/no-lockfile branch regressed to NodejsNpmInstallAction.
Pass experimental_flags=["experimentalNodejsMonorepo"].
| LOG.debug("NODEJS could not resolve the npm project root of %s: %s", install_dir, ex) | ||
| return None | ||
|
|
||
| for lockfile_name in ("package-lock.json", "npm-shrinkwrap.json"): |
There was a problem hiding this comment.
[GENERAL] The lookup order contradicts npm's own precedence. npm prefers npm-shrinkwrap.json over package-lock.json when both exist in a package root, but this loop returns package-lock.json first:
for lockfile_name in ("package-lock.json", "npm-shrinkwrap.json"):No live bug today because the caller only uses the result for truthiness, but the docstring promises "the lockfile npm would use" / "Path of the lockfile in npm's project root", and this is a public static method that the existing LOCKFILE_TYPES integration matrix already builds package-lock-and-shrinkwrap fixtures for. Swapping the tuple order makes the returned path match what the install will actually read:
for lockfile_name in ("npm-shrinkwrap.json", "package-lock.json"):Notes on scope: lock files and the new JSON/JS fixtures were read for consistency only (the file:../npm-deps entry in with-local-dependency-and-lockfile/package-lock.json matches the real npm-deps manifest, and the workspaces fixture's v2 lockfile is internally consistent). I did not run any code.
2 participants
Issue #, if available: reported in aws/aws-sam-cli#6567
no linked issue: aws/aws-sam-cli#6567 asks for more than this fix, so this PR must not close it
Description of changes
Building nodejs dependencies in source ignored the project's lockfile: the install step ran
npm updatewith--no-package-lock, so every build re-resolved the ranges inpackage.jsonand could upgrade dependencies in the developer's own source tree. On a workspaces project whose rootpackage-lock.jsonpinslodash4.17.20 (manifest^4.17.20), onesam build --build-in-sourceleft 4.18.1 installed.Opt-in while it rolls out.
is_nodejs_monorepo_support_enabledreadsexperimentalNodejsMonorepofrom theexperimental_flagsthe workflow is constructed with - sam-cli'sExperimentalFlag.NodejsMonorepo, whose config key crosses the boundary as that string, the wayexperimentalBuildPerformancealready does forpython_pip. Without the flag every in-source build keepsnpm update --no-package-lock, and npm is not even asked where the lockfile is.npm update --no-audit --no-save --omit=dev --no-package-lock --install-linksnpm install -q --no-audit --no-save --omit=dev --install-linksnpm update ...Which directory holds the lockfile npm will use is npm's own decision, so
NodejsNpmWorkflow.get_lockfile_path()asks it:npm prefix, run in the install directory, reports the monorepo root for a workspace package and the package itself for any other nested package, whose ancestors' lockfiles npm ignores. Identical on npm 8.19.4, 9.9.4, 10.9.9 and 11.19.0.NodejsNpmInstallActiontakes back aninstall_linksargument.Nothing else moves.
npm cistays gated on a lockfile in the source directory itself — run from a workspace package it empties the workspace root's devDependencies on every npm version.Relation to #579
This PR moves the lockfile case back onto
npm install, and #579 deliberately moved build-in-source offnpm installontonpm update— so it is worth being explicit that this does not undo that fix.What #579 fixed: with
--build-in-source, a dependency removed frompackage.jsonbetween two builds was left behind in the source tree'snode_modules. Its fix introducedNodejsNpmUpdateAction.The pruning #579 wanted comes from npm's reify step, which both commands run — not from the choice between
updateandinstall. What made the lockfile get ignored is the--no-package-lockflag that came along withnpm update, and that flag is the only thing this PR drops:npm install ... --install-links, which reconcilesnode_modulesagainst the manifest and the lockfile, pruning what no longer belongs.NodejsNpmUpdateAction, fix: use npm update when building in source #579's own action, untouched.Two integration tests pin this, one per path, both added here:
test_build_in_source_with_removed_dependencies_and_a_lockfile— builds, removes the dependency from the manifest, rebuilds, asserts it is gone from the source tree, and asserts the developer's lockfile is byte-identical afterwards, including once it is out of date with the manifest.test_build_in_source_with_removed_dependencies— the same property on the lockfile-lessnpm updatepath, so fix: use npm update when building in source #579's behaviour keeps a test of its own rather than resting on assumption.Both pass on every npm major listed below, and were re-confirmed on npm 10.9.9 and 11.19.0 specifically for this question (10 tests on each, across all supported runtimes). So a project with a lockfile gains the pinning and loses nothing #579 gave it.
Performance.
npm update --no-package-lockre-resolves the whole workspace tree once per function; the lockfile install on an already-consistent tree is close to a no-op. On an npm-workspaces monorepo with disjoint per-function dependencies (~300 packages, esbuild functions,--build-in-source, median of 3 full builds):One pre-existing gap stays open: in a monorepo the plain workflow's artifact link finds no
node_modulesbeside the function (npm hoists to the root) and skips silently — tracked as #933 and named in a comment at the link site. The esbuild workflow is unaffected, since it bundles dependencies into the output file.The new manifests and lockfiles under the
nodejs_npm/nodejs_npm_esbuildintegrationtestdata/directories are test fixture data, not a dependency change to this package.Description of how you validated changes
Five new integration tests; four fail on
developwithAssertionError: '1.5.0' != '1.3.0'(every fixture's lockfile pinsminimal-request-promise1.3.0 while its manifest allows^1.3.0). They cover: the workspaces monorepo (root lockfile; the produced bundle is required withnode, so the artifact is covered and not just the installed tree; the root's workspace link stays a link to the developer'spackages/fn), removed-dependency pruning with and without a lockfile (see Relation to #579 above), a version 1 lockfile (npm 6 format), afile:local dependency under--install-links(lands as a real directory, not a symlink out of the artifacts), and devDependency pruning from a pre-populatednode_modules. Four of them read the lockfile bytes before the build and assert they are unchanged after it, atlockfileVersion1, 2 and 3 — dropping--no-saverewrites the version 1 fixture and fails.The new tests construct the workflow with the flag, since they assert the behaviour it gates;
test_without_the_flag_a_lockfile_is_ignored_and_the_update_runsis the other side - a project whose lockfile npm would read still takes the update, andnpm prefixis never called.New unit tests cover the lockfile lookup against a stubbed
npm prefix(project root, workspaces root, shrinkwrap, absent, an ancestor's lockfile ignored for a nested non-workspace package,npm prefixfailing), the install-directory semantics, and--install-linkson the install action. The existing build-in-source wiring tests pin the no-lockfile case tonpm update.make prequivalent: 958 passed, coverage 94.67%;ruff checkandblack --checkclean.npm prefixare npm-version dependent, the lockfile subset was re-run on each npm major CI builds — 8.19.4, 9.9.4, 10.9.9, 11.19.0 — 20 passed on every one. Linux only locally; the Windows lanes are untested outside CI.NpmInstallinstead ofNpmUpdate, rootlodashstays at the locked 4.17.20 across repeat builds (4.18.1 before), the rootesbuilddevDependency survives, and the bundle still inlines the local workspace package.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.