fix(lambda-nodejs): do not require a frozen lockfile for bun #32908

blimmer · 2025-01-14T00:59:49Z

Issue #32906

Reason for this change

When I implemented bun support, I accidentally used --frozen-lockfile, which caused issues when the lockfile contained additional entries other than what's being bundled in the docker container.

The issue has a small repro-case.

Description of changes

I removed the --frozen-lockfile flag, which resolved the problem.

Describe any new or updated permissions being added

N/A

Description of how you validated changes

I added an additional package to the bun.lockb file in the integration test. This caused the issue to occur in the test suite. Once I made the changes in this PR, the test started passing again.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

blimmer · 2025-01-14T01:09:06Z

packages/@aws-cdk-testing/framework-integ/test/aws-lambda-nodejs/test/integ.dependencies-bun.ts

@@ -10,7 +10,7 @@ const stack = new cdk.Stack(app, 'TestStack');

 const handler = new lambda.NodejsFunction(stack, 'Function', {
  entry: path.join(__dirname, 'integ-handlers/bun/dependencies-bun.ts'),
-  runtime: Runtime.NODEJS_20_X,
+  runtime: Runtime.NODEJS_22_X,


Just bumped this to make the PR linter happy and to force re-run the integration test.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

codecov · 2025-01-14T01:26:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.21%. Comparing base (878ad54) to head (3098560).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32908   +/-   ##
=======================================
  Coverage   82.21%   82.21%           
=======================================
  Files         119      119           
  Lines        6876     6876           
  Branches     1162     1162           
=======================================
  Hits         5653     5653           
  Misses       1120     1120           
  Partials      103      103

Flag	Coverage Δ
suite.unit	`82.21% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`∅ <ø> (∅)`
packages/aws-cdk-lib/core	`82.21% <ø> (ø)`

aws-cdk-automation · 2025-02-02T00:13:46Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

blimmer · 2025-02-02T18:25:00Z

...aws-cdk-testing/framework-integ/test/aws-lambda-nodejs/test/integ-handlers/bun/_package.json

@@ -0,0 +1,8 @@
+{


cc @rix0rrr - I see that you were working with this setup in #33022. Does this setup seem reasonable to you? I need to make sure that the lockfile will be modified during bundling to avoid the origin issue this PR fixes.

Hey @blimmer,

It seems as reasonable as anything. I honestly don't quite know what the best way is to go about updating this test, I know it gave me a lot of grief in my previous PR.

I confess I haven't dived into the possible solutions too deeply, so you have more context than I do on this right now. If you think this is the way to go, I'll happily give it a shot!

Gotcha. I TDD'ed this, and before my change, this test was broken. So I think this is as good as anything 👍

blimmer · 2025-02-02T18:40:56Z

It looks like the codecoverage collection step is failing on all PRs right now.

samson-keung · 2025-02-28T22:42:13Z

packages/aws-cdk-lib/aws-lambda-nodejs/lib/package-manager.ts

-          installCommand: logLevel && logLevel !== LogLevel.INFO ? ['bun', 'install', '--frozen-lockfile', '--backend', 'copyfile', '--silent'] : ['bun', 'install', '--frozen-lockfile', '--backend', 'copyfile'],
+          // Bun's default is to not force `--frozen-lockfile`, so it's not specified here. If they ever add a
+          // flag to explicitly disable it, we should add it here. https://github.com/oven-sh/bun/issues/16387
+          installCommand: logLevel && logLevel !== LogLevel.INFO ? ['bun', 'install', '--backend', 'copyfile', '--silent'] : ['bun', 'install', '--backend', 'copyfile'],


@blimmer Thanks for continuing to work on this. I am not familiar with bun so I have a few questions hoping you can help.

It seems that bun is in early stage and looks like they changed the lockfile to be text based now (bun's blog). As they evolve, I expect more changes like these in the future. Should we mark bun support in CDK as experimental for now? Because otherwise, we will have to maintain backward compatibility for old bun behaviour.

Will removing --frozen-lockfile have impact to CDK users who rely on or expects having --frozen-lockfile on? I see @suds-sky was interested in the original bun support feature requests. @suds-sky maybe you would have some input as well? I worry about this being a potential breaking change.

Hey @samson-keung - thank you for your review.

Experimental?

It seems that bun is in early stage and looks like they changed the lockfile to be text based now (bun's blog). As they evolve, I expect more changes like these in the future. Should we mark bun support in CDK as experimental for now? Because otherwise, we will have to maintain backward compatibility for old bun behaviour.

Yes, bun is evolving rapidly. I filed #33270 a while back to support the text-based lockfile. From the linked blog:

We're planning to make bun.lock the default in Bun v1.2.0. In the meantime, we continue to support the binary bun.lockb format and will do so for awhile.

So it sounds like, for proper CDK support, we'd want to identify and support both. I did not tackle that as part of this PR, though, since it would increase scope and the behavior today is broken for most people.

I'm not very familiar with experimental features, so I'm open to additional ideas/discussion of this!

Affecting Existing Users?

Will removing --frozen-lockfile have impact to CDK users who rely on or expects having --frozen-lockfile on? I see @suds-sky was interested in the original bun support feature requests. @suds-sky maybe you would have some input as well? I worry about this being a potential breaking change.

I feel confident that this is not a breaking change for existing users of this feature. This bug is caused by having a package.json/bun.lockb file with additional packages from what's being bundled in-container via the nodeModules property.

When a nodeModules array is passed, this logic executes:

aws-cdk/packages/aws-cdk-lib/aws-lambda-nodejs/lib/bundling.ts

Lines 285 to 294 in 7538a84

// Create dummy package.json, copy lock file if any and then install

depsCommand = chain([

isPnpm ? osCommand.write(pathJoin(options.outputDir, 'pnpm-workspace.yaml'), ''): '', // Ensure node_modules directory is installed locally by creating local 'pnpm-workspace.yaml' file

osCommand.writeJson(pathJoin(options.outputDir, 'package.json'), { dependencies }),

osCommand.copy(lockFilePath, pathJoin(options.outputDir, this.packageManager.lockFile)),

osCommand.changeDirectory(options.outputDir),

this.packageManager.installCommand.join(' '),

isPnpm ? osCommand.remove(pathJoin(options.outputDir, 'node_modules', '.modules.yaml'), true) : '', // Remove '.modules.yaml' file which changes on each deployment

isBun ? osCommand.removeDir(pathJoin(options.outputDir, 'node_modules', '.cache')) : '', // Remove node_modules/.cache folder since you can't disable its creation

]);

As part of that logic, the lockfile is copied over, and bun install <packagename> is run. But, because the lockfile contains other dependencies, the error is thrown.

This is why we don't enforce a frozen lockfile on any other package managers.

So, the only way this feature would have worked for existing users is if:

a) they didn't specify any nodeModules, or;
b) their lockfile only contained the packages passed as nodeModules.

Therefore, I think it's OK to make this change without breaking existing users.

Experimental?

Thank you for sharing your thoughts. I don't have historical context on how CDK typically add experimental features either. I will check with the team. But this shouldn't be a blocker on this PR.

Affecting Existing Users?

Thank you for the clear explanation so I can understand how this part work! I agree that it shouldn't be a breaking change.

Pull request has been modified.

samson-keung · 2025-03-03T20:50:02Z

packages/aws-cdk-lib/aws-lambda-nodejs/lib/package-manager.ts

-          installCommand: logLevel && logLevel !== LogLevel.INFO ? ['bun', 'install', '--frozen-lockfile', '--backend', 'copyfile', '--silent'] : ['bun', 'install', '--frozen-lockfile', '--backend', 'copyfile'],
+          // Bun's default is to not force `--frozen-lockfile`, so it's not specified here. If they ever add a
+          // flag to explicitly disable it, we should add it here. https://github.com/oven-sh/bun/issues/16387
+          installCommand: logLevel && logLevel !== LogLevel.INFO ? ['bun', 'install', '--backend', 'copyfile', '--silent'] : ['bun', 'install', '--backend', 'copyfile'],


Experimental?

Thank you for sharing your thoughts. I don't have historical context on how CDK typically add experimental features either. I will check with the team. But this shouldn't be a blocker on this PR.

Affecting Existing Users?

Thank you for the clear explanation so I can understand how this part work! I agree that it shouldn't be a breaking change.

mergify · 2025-03-03T20:51:15Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2025-03-03T20:51:20Z

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

Pull request has been modified.

aws-cdk-automation · 2025-03-03T21:23:36Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 3098560
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2025-03-03T21:31:34Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2025-03-03T21:31:51Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

aws-cdk-automation requested a review from a team January 14, 2025 01:00

github-actions bot added bug This issue is a bug. p2 admired-contributor [Pilot] contributed between 13-24 PRs to the CDK labels Jan 14, 2025

aws-cdk-automation previously requested changes Jan 14, 2025

View reviewed changes

blimmer changed the title ~~fix(aws-lambda-nodejs): do not require a frozen lockfile for bun~~ fix(lambda-nodejs): do not require a frozen lockfile for bun Jan 14, 2025

blimmer commented Jan 14, 2025

View reviewed changes

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 14, 2025

blimmer mentioned this pull request Jan 14, 2025

(aws-lambda-nodejs): bun depenedencies fail with error: lockfile had changes, but lockfile is frozen #32906

Closed

1 task

github-actions bot mentioned this pull request Feb 1, 2025

Monthly PR metrics report #33266

Closed

blimmer added 5 commits February 2, 2025 10:13

fix: do not require a frozen lockfile for bun

ccbd891

chore: update bun.lockb

d5184d3

chore: bump the runtime to force the test to re-run

21196f5

docs: add comment about regenerating the lockfile

2962dea

fix: update integration tests

b6e3738

blimmer force-pushed the fix-bun branch from 14f5bba to b6e3738 Compare February 2, 2025 18:05

fix: smaller package.json

fbb115c

blimmer commented Feb 2, 2025

View reviewed changes

Merge branch 'main' into fix-bun

a5303ab

samson-keung previously requested changes Feb 28, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 28, 2025

blimmer requested a review from samson-keung March 3, 2025 00:27

Merge branch 'main' into fix-bun

fbaef92

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Mar 3, 2025

samson-keung previously approved these changes Mar 3, 2025

View reviewed changes

samson-keung self-assigned this Mar 3, 2025

Merge branch 'main' into fix-bun

3098560

samson-keung approved these changes Mar 3, 2025

View reviewed changes

mergify bot merged commit a21190e into aws:main Mar 3, 2025
22 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2025

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Mar 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(lambda-nodejs): do not require a frozen lockfile for bun #32908

fix(lambda-nodejs): do not require a frozen lockfile for bun #32908

blimmer commented Jan 14, 2025

aws-cdk-automation left a comment

blimmer Jan 14, 2025

codecov bot commented Jan 14, 2025 •

edited

Loading

aws-cdk-automation commented Feb 2, 2025

blimmer Feb 2, 2025

rix0rrr Feb 28, 2025

blimmer Mar 3, 2025

blimmer commented Feb 2, 2025

samson-keung Feb 28, 2025

blimmer Mar 3, 2025

samson-keung Mar 3, 2025

samson-keung Mar 3, 2025

mergify bot commented Mar 3, 2025

mergify bot commented Mar 3, 2025

aws-cdk-automation commented Mar 3, 2025

mergify bot commented Mar 3, 2025

github-actions bot commented Mar 3, 2025

	// Create dummy package.json, copy lock file if any and then install
	depsCommand = chain([
	isPnpm ? osCommand.write(pathJoin(options.outputDir, 'pnpm-workspace.yaml'), ''): '', // Ensure node_modules directory is installed locally by creating local 'pnpm-workspace.yaml' file
	osCommand.writeJson(pathJoin(options.outputDir, 'package.json'), { dependencies }),
	osCommand.copy(lockFilePath, pathJoin(options.outputDir, this.packageManager.lockFile)),
	osCommand.changeDirectory(options.outputDir),
	this.packageManager.installCommand.join(' '),
	isPnpm ? osCommand.remove(pathJoin(options.outputDir, 'node_modules', '.modules.yaml'), true) : '', // Remove '.modules.yaml' file which changes on each deployment
	isBun ? osCommand.removeDir(pathJoin(options.outputDir, 'node_modules', '.cache')) : '', // Remove node_modules/.cache folder since you can't disable its creation
	]);

fix(lambda-nodejs): do not require a frozen lockfile for bun #32908

fix(lambda-nodejs): do not require a frozen lockfile for bun #32908

Conversation

blimmer commented Jan 14, 2025

Issue #32906

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 14, 2025 • edited Loading

Codecov Report

aws-cdk-automation commented Feb 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blimmer commented Feb 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Experimental?

Affecting Existing Users?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Mar 3, 2025

mergify bot commented Mar 3, 2025

aws-cdk-automation commented Mar 3, 2025

AWS CodeBuild CI Report

mergify bot commented Mar 3, 2025

github-actions bot commented Mar 3, 2025

codecov bot commented Jan 14, 2025 •

edited

Loading