Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(aws-lambda-nodejs): Support bun lockfiles for NodejsFunction bundling #31753

Closed
1 of 2 tasks
blimmer opened this issue Oct 14, 2024 · 6 comments
Closed
1 of 2 tasks

(aws-lambda-nodejs): Support bun lockfiles for NodejsFunction bundling #31753

blimmer opened this issue Oct 14, 2024 · 6 comments
Labels
@aws-cdk/aws-lambda-nodejs effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2

Comments

@blimmer
Copy link
Contributor

blimmer commented Oct 14, 2024 •
edited
Loading

Describe the feature

Today, NodejsFunction supports lockfiles from the following ecosystems:

  • NPM
  • Yarn
  • PNPM

bun is gaining in popularity and has its own lockfile, bun.lockb. It would be great if NodejsFunction supported using this lockfile.

Use Case

We've been migrating our apps to use bun since it's so much faster than node. However, when creating a new NodejsFunction with a bun lockfile, we get this error:

Error: Cannot find a package lock file (pnpm-lock.yaml, yarn.lock or package-lock.json). Please specify it with depsLockFilePath.

For now, we'll probably move over to a supported package manager, but we'd love to see native support for bun.lockb.

Proposed Solution

Update the bundling logic to include bun and support bun install-ing.

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.162.1

Environment details (OS name and version, etc.)

MacOS
@blimmer blimmer added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Oct 14, 2024
@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Oct 14, 2024
@khushail khushail self-assigned this Oct 14, 2024
@khushail
Copy link
Contributor

Thanks @blimmer for submitting the feature request.

@khushail khushail added effort/medium Medium work item – several days of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Oct 14, 2024
@khushail khushail removed their assignment Oct 14, 2024
@khushail khushail added the p2 label Oct 14, 2024
@blimmer blimmer mentioned this issue Oct 15, 2024
Closed
1 task
@blimmer
Copy link
Contributor Author

blimmer commented Oct 15, 2024

Thanks @khushail - I've got a PR ready for review here: #31770

@epoctic
Copy link

epoctic commented Oct 16, 2024 •
edited
Loading

I don't recommend developing using Bun and deploying to a Node platform. While I'm sure you'd be fine in the vast majority of cases, it could lead to some particularly annoying bugs.

Maybe I'm missing something, but it seems like this could only lead to confusion if users expect lock file support to imply Bun engine use.

Edit: After thinking on it a bit more, I think you'd likely be okay. The issues I've seen were related to moving Node projects to Bun, rather than the other way around.

@blimmer
Copy link
Contributor Author

blimmer commented Oct 16, 2024

This logic just allows installing dependencies from package.json when you have a bun lockfile. Since bun uses the standard node_modules resolution of packages, I think we'd be OK in the vast majority of cases.

@github-actions github-actions bot mentioned this issue Nov 1, 2024
Closed
mergify bot pushed a commit to blimmer/aws-cdk that referenced this issue Dec 5, 2024
@blimmer
feat(lambda-nodejs): add bun support (aws#31770)
aed8ad1 
### Issue aws#31753

Closes aws#31753.

### Reason for this change

`bun` is rapidly gaining popularity because it's extremely performant compared to `node`. `bun` has its own lockfile, `bun.lockb`, which is not currently respected by AWS CDK when bundling `NodejsFunction`s.

### Description of changes

This code is very well-structured, so it was simple to add bun support alongside `yarn`, `pnpm` and `npm`.

### Description of how you validated changes

I linked this code up to a simple, sample CDK app with a `bun` lockfile. I wasn't able to `bun cdk synth` before my changes. Then, after linking up my local workspace, I was able to bundle with my `bun` lockfile.

I also added unit tests similar to those that exist for the other packages managers.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@blimmer
Copy link
Contributor Author

blimmer commented Dec 5, 2024

Something really weird happened with the PR. The commit ended up on main (aed8ad1), but the PR didn't get marked as merged. So, this is fixed and should out soon 👍

@blimmer blimmer closed this as completed Dec 5, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 5, 2024

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
@aws-cdk/aws-lambda-nodejs effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(lambda-nodejs): add bun support blimmer/aws-cdk
3 participants
@blimmer @epoctic @khushail