-
Notifications
You must be signed in to change notification settings - Fork 394
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.
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
feat: Add Bun as supported package manager #2223
base: master
Are you sure you want to change the base?
Conversation
Does bun support Nest now? Last I knew (which admittedly was a while ago), bun had some issues running Nest applications |
It does! Our Node.js compatibility has come a long way in recent months, and more and more frameworks are working out of the box. Our recent support for Nest is the reason for my creating this PR. Note that the command printed after If you want to see Nestjs working with the Bun runtime, create a new project with $ bunx nest start
[Nest] 9357 - 08/07/2023, 3:07:55 PM LOG [NestFactory] Starting Nest application...
[Nest] 9357 - 08/07/2023, 3:07:55 PM LOG [InstanceLoader] AppModule dependencies initialized +5ms
[Nest] 9357 - 08/07/2023, 3:07:55 PM LOG [RoutesResolver] AppController {/}: +5ms
[Nest] 9357 - 08/07/2023, 3:07:55 PM LOG [RouterExplorer] Mapped {/, GET} route +1ms
[Nest] 9357 - 08/07/2023, 3:07:55 PM LOG [NestApplication] Nest application successfully started +1ms Question, am I supposed to be able to see the output of the CI run? The test suite is passing for me locally. |
I did give it a quick test with creating a As it turns out, my project uses a few packages that bun does not support yet (node's crypto package), but otherwise it seemed like everything would have worked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Overall, it seems bun currently supports Nest, so long as some of Node's built in packages (like generateKey
from crypto
and others) aren't used it'll all work just fine. It should be noted that the repl
package isn't supported yet either, but that's opt-in so we can make a warning in the docs
Awesome thanks Jay! Those incompatibilities are called out in Bun's Node.js compat page: https://bun.sh/docs/runtime/nodejs-apis. By and large, Bun users aware of the fact they're using a pre-stable runtime. Needless to say, feel free to close any issues that are mistakenly opened in the Nest.js repo, but anecdotally Bun users usually know to blame Bun when they encounter Node.js compatibility issues. Happy to revert this if it increases your maintenance burden. |
I hope that this remains true. Speaking from my experience, we get a lot of issues about library issues that Nest just wraps with small modules where the issue should be reported to the underlying module, so I'd be expecting to see issues show up either in GitHub or Discord, but we'll definitely direct them to the correct place 😄 |
@jmcdo29 Oops I made a mistake - |
Fixed CI - forgot to |
With #2228 landed this and nestjs/docs.nestjs.com#2820 should be good to merge We're hoping to announce Nest.js support in an upcoming release blog post as well 👍 |
Excellent work! Thank you very much for these advances! This is great ! |
@kamilmysliwiec Sorry about that. I could put in another PR to handle absolute paths there but we've since fixed this on the Bun side as well: oven-sh/bun#4113 Reverting that change does not affect Bun compatibility (for Bun v0.8+) and this PR remains mergeable. |
So excited for this combination |
Other problem of |
@v1d3rm3 Nest doesn't use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I'd actually like to revoke my approval for this: the reason is that Bun itself is not 100% compatible with Nest. Mainly the need for supporting emitDecoratorMetadata
. I understand that as a package manager alone Bun is fine, but I can guarantee that if we add Bun as a "supported package manager", there will be swarms of people that see it as "supported runtimes" even though npm
, yarn
, and pnpm
are not runtimes themselves. It will almost surely increase noisy tickets about Bun not working with Nest, when that's not the aim of this addition.
If people would like to use Bun as a package manager with Nest, then more power to them, they can either erase the ndoe_modules
or use the --skip-install
flag, like we suggested with pnpm before adding it to this list.
I do hope to see this be an eventual feature, but until I know that we won't get issues about Bun as a runtime, when we only add it as a package manager, I wouldn't feel comfortable merging this in.
fair enough! so I guess we should merge this only after they resolve this issue on their side: oven-sh/bun#1641 |
Bun now supports
It now seems that the emitted metadata is indeed correctly there and working to allow Bun to be bother a package manager and a runtime environment for Nest. Do be aware that there are some quirks with fastify, and pino, so it's technically only a partial support. |
Hello @jmcdo29 With node a small project: ±150 mb |
Didn't check for memory consumption, was more concerned if it worked in the first place |
You guys most probably saw this comment made by Jarred on the Bun project but in case you didn't...
|
Hi, what is this currently waiting for? @jmcdo29 It's not clear to me if this is waiting for someone/something to be done or it's ready to be merged now? |
I tried to run smallest microservice on bun, but it gives errors, one of them I posted earlier. |
@vnenkpet Bun as a package manager does indeed work with Nest. Bun as a runtime mostly works, but there are still quirks with pino and fastify, as I've already mentioned. If we are to merge this in right now, I would want to be incredibly clear in the prompt that this is for Bun the package manager and not Bun the runtime. While (probably) most people who use Bun know to report errors to the Bun runtime repo, I have no doubt in my mind that we'd still get a good number of Bun related bugs that having nothing explicitly to do with Nest which would end up generating a lot of noise for us. So for right now, I want to wait for Bun to stabilize a bit more. I want to know that we won't be getting issues for the Bun runtime in our repos, I want to know that we won't end up having an uptick in our Discord server for Bun support, and I want to know that Bun supports Nest with both Express and Fastify, and whatever logger people end up choosing to use (winston, bunyan, pino, ogma, etc). If anyone wants to use Bun with Nest and see how it goes, great! Please do and support the ecosystem and the Bun community, but be aware that any bugs are not inherently on Nest to fix, and should be properly reported to the correct place. |
This has partially been fixed here https://bun.sh/blog/bun-v1.0.17 |
@kamilmysliwiec what's blocking this PR from being merged? |
I am using bun as a package manager (not as a runtime) for over a year now within all my projects and never had a single issue. Even the hosting providers I use (Vercel, Cloudflare, Render) all support Bun as a package manager. The PR is nearly a year old and is pretty straight forward. Can it be merged? @kamilmysliwiec |
@MickL as soon as they'll be full Node - Bun parity and using Bun as a runtime is going to be fully safe, we'll merge this PR |
Why is there a need to wait for the runtime? This PR is just about replacing the package manager, not the runtime. |
Because it's quite likely that some people may think they can use Bun as a runtime too, if it's one of the suggested options in the CLI. |
I dont think this will be the case. The CLI adds scripts that clearly use Node |
Please merge it |
Associated docs PR: nestjs/docs.nestjs.com#2820
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Adds
bun
as supported package manager. Bun is a toolkit that includes a runtime-agnostic package manager that can be used in Node.js or Bun projects.What is the new behavior?
Bun is supported in
nest new --package-manager
. Also the Bun lockfilebun.lockb
is auto-detected and used to pick the default package manager.Does this PR introduce a breaking change?
Other information
Below are some successful runs on my local machine.
CLI flag
Inference based on
bun.lockb
: