Skip to content

npm: use "ci" (cleaninstall) if lockfiles are present#580

Merged
floydspace merged 1 commit intofloydspace:masterfrom
benknoble:npm-ci
Dec 20, 2025
Merged

npm: use "ci" (cleaninstall) if lockfiles are present#580
floydspace merged 1 commit intofloydspace:masterfrom
benknoble:npm-ci

Conversation

@benknoble
Copy link
Copy Markdown
Contributor

At work, we sometimes find that sls package fails because npm has resolved a
newer version than is available in our mirrors; as best as we can tell, this is
because the esbuild plugin is running npm install instead of npm ci. But we
want a fast, reproducible, known build, favoring the latter. This PR implements
that (see the commit message for details).

If there's a way to make this change non-breaking, I'm all ears. It seems
reasonable to want to toggle the behavior using the existing configuration.

@benknoble
Copy link
Copy Markdown
Contributor Author

range-diff: v2: reformat and propagate constructor argument
1:  b59a623 ! 1:  2f3a91c npm: use "ci" (cleaninstall) if lockfiles are present
    @@ README.md: #### Default Esbuild Options
      #### Watch Options
      
     
    + ## src/packagers/index.ts ##
    +@@ src/packagers/index.ts: import type { PackagerId, PackagerOptions } from '../types';
    + import type { Packager } from './packager';
    + 
    + const packagerFactories: Record<PackagerId, (packagerOptions: PackagerOptions) => Promise<Packager>> = {
    +-  async npm() {
    ++  async npm(packagerOptions: PackagerOptions) {
    +     const { NPM } = await import('./npm');
    + 
    +-    return new NPM();
    ++    return new NPM(packagerOptions);
    +   },
    +   async pnpm() {
    +     const { Pnpm } = await import('./pnpm');
    +
      ## src/packagers/npm.ts ##
     @@ src/packagers/npm.ts: import { Predicate } from 'effect';
      import { any, isEmpty, replace, split, startsWith, takeWhile } from 'ramda';
    @@ src/packagers/npm.ts: export class NPM implements Packager {
          const command = /^win/.test(process.platform) ? 'npm.cmd' : 'npm';
      
     -    const args = ['install', ...extraArgs];
    -+    const args = [
    -+      !this.packagerOptions.ignoreLockfile && useLockfile ? 'ci' : 'install',
    -+      ...extraArgs
    -+    ];
    ++    const args = [!this.packagerOptions.ignoreLockfile && useLockfile ? 'ci' : 'install', ...extraArgs];
      
          await spawnProcess(command, args, { cwd });
        }

Copy link
Copy Markdown
Owner

@floydspace floydspace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls check failing tests

@benknoble
Copy link
Copy Markdown
Contributor Author

Thanks! I don't have a full node install setup locally at the moment, so if there's any way for me to use the CI here more readily, that might help.

Otherwise I'll keep at it and see what I can do.

Lockfiles, explicitly designed for reproducible builds, are ignored by
the npm packager implementation of the plugin. Thus `sls package` will
attempt to install different versions of dependencies after resolving
them if it can (due to `npm install`).

This makes it harder to achieve reproducible builds and reliable
deployments. When a lockfile is present for npm, use the cleaninstall
command instead to install from the lockfile (also saving time on
resolution). Use the pattern in the Yarn packager implementation,
allowing this behavior to be tweaked by the use of packager options
`ignoreLockfile`. Since that configuration defaults to `false` [1], this
is a breaking change for npm users.

[1]: https://www.serverless.com/plugins/serverless-esbuild#packager-options
@benknoble
Copy link
Copy Markdown
Contributor Author

Ok, I managed to set things up locally and run tests.

range-diff: v3: fix tests
1:  2f3a91c ! 1:  869452d npm: use "ci" (cleaninstall) if lockfiles are present
    @@ src/packagers/npm.ts: export class NPM implements Packager {
      
          await spawnProcess(command, args, { cwd });
        }
    +
    + ## src/tests/packagers/npm.test.ts ##
    +@@ src/tests/packagers/npm.test.ts: import * as utils from '../../utils';
    + 
    + jest.mock('process');
    + describe('NPM Packager', () => {
    +-  const npm = new NPM();
    ++  const npm = new NPM({});
    +   const path = './';
    + 
    +   let spawnSpy: jest.SpyInstance;

However, npm run test:e2e fails with

Error:
Error [ERR_REQUIRE_ASYNC_MODULE]: require() cannot be used on an ESM graph with top-level await. Use import() instead. To see where the top-level await comes from, use --experimental-print-required-tla.
    at ModuleJobSync.runSync (node:internal/modules/esm/module_job:392:13)
    at ModuleLoader.importSyncForRequire (node:internal/modules/esm/loader:329:47)
    at loadESMFromCJS (node:internal/modules/cjs/loader:1411:24)
    at Module._compile (node:internal/modules/cjs/loader:1544:5)
    at Object..js (node:internal/modules/cjs/loader:1668:16)
    at Module.load (node:internal/modules/cjs/loader:1313:32)
    at Function._load (node:internal/modules/cjs/loader:1123:12)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:217:24)
    at Module.require (node:internal/modules/cjs/loader:1335:12)
    at require (node:internal/modules/helpers:136:16)
    at module.exports (/home/benknoble/code/serverless-framework-esbuild/.test-artifacts/minimal/node_modules/serverless/lib/utils/require-with-import-fallback.js:5:17)
    at PluginManager.requireServicePlugin (/home/benknoble/code/serverless-framework-esbuild/.test-artifacts/minimal/node_modules/serverless/lib/classes/plugin-manager.js:171:14)
    at PluginManager.resolveServicePlugins (/home/benknoble/code/serverless-framework-esbuild/.test-artifacts/minimal/node_modules/serverless/lib/classes/plugin-manager.js:198:29)
    at async PluginManager.loadAllPlugins (/home/benknoble/code/serverless-framework-esbuild/.test-artifacts/minimal/node_modules/serverless/lib/classes/plugin-manager.js:136:36)
    at async Serverless.init (/home/benknoble/code/serverless-framework-esbuild/.test-artifacts/minimal/node_modules/serverless/lib/serverless.js:146:5)
    at async /home/benknoble/code/serverless-framework-esbuild/.test-artifacts/minimal/node_modules/serverless/scripts/serverless.js:601:7

🤷

@floydspace
Copy link
Copy Markdown
Owner

No worries, pipeline runs e2e tests and they seem green. So I consider it is all sorted

@floydspace floydspace merged commit ccac4a1 into floydspace:master Dec 20, 2025
4 checks passed
@benknoble benknoble deleted the npm-ci branch December 20, 2025 22:52
@github-actions
Copy link
Copy Markdown

🎉 This PR is included in version 1.57.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants