node-gradle / gradle-node-plugin

Gradle plugin for integrating NodeJS in your build. :rocket:
Apache License 2.0
599 stars 117 forks source link

Bun support #290

Closed harrel56 closed 10 months ago

harrel56 commented 10 months ago

Hi, I was thinking about developing a standalone bun gradle plugin, but stumbled upon this issue (https://github.com/node-gradle/gradle-node-plugin/issues/288) and thought that I can give it a try.

For now, it's WIP but I tested it out and installing bun and running it somewhat works. Still, there probably will be a lot more stuff to be done. If you have any suggestion/pointers I will gladly take any advice - at this state it's basically a copy of pnpm/yarn setup.

And there is one major issue: bun is not yet supported on windows :( Normally I would work around it by using WSL but I'm not sure how's that gonna play with the rest of the code. So I guess windows support would be out of scope in this PR

deepy commented 10 months ago

At a glance this looks good, but we're gonna need a test to verify the functionality, I think adapting result1 and result2 from this test should get you most of the way https://github.com/node-gradle/gradle-node-plugin/blob/master/src/test/groovy/com/github/gradle/node/npm/task/NpmTask_integTest.groovy#L15

harrel56 commented 10 months ago

Yeah, gonna add tests for sure. Also gonna add BunInstallTask and BunxTask. Should I add task rules for bun too?

deepy commented 10 months ago

I'd skip the task rules, the personal experience I've got with them is that they're good for build engineers (who don't like them) and absolutely awful for average users (who loves them) The short of it is that it makes people expect npm_install to be the correct task and then they get unexpected results

harrel56 commented 10 months ago

Alright, I think this should be somewhat complete. I would expect that some GH actions could fail on different OSes - if that happens I'm gonna try fix it asap. Let me know if there's anything else to be done :)

harrel56 commented 10 months ago

Also all the Install tasks are kind of funky - personally would separate BunInstall & BunCi (which is just install with --frozen-lockfile), but I don't know if it would make sense in a real world CI/build flow. Ofc anyone could write the Install task by themselves in a way that it suits them, so I guess it's not a big deal

deepy commented 10 months ago

Conditional tasks gets a little weird when you want to depend on them, and from experience it seems most people use the install tasks anyways (mostly with ephemeral environments)

I'm gonna have time to review this properly on Sunday or during next week, but at a glance it looks good, if it's merged I may change the API. But there's one major outstanding question, given that the Windows support doesn't yet exist in Bun do you imagine that people using this would expect to wire up potential Windows support themselves or want a convenience-method for doing the wiring?

harrel56 commented 10 months ago

But there's one major outstanding question, given that the Windows support doesn't yet exist in Bun do you imagine that people using this would expect to wire up potential Windows support themselves or want a convenience-method for doing the wiring?

I have a hard time imaging how it could work on Windows and how users could make bun tasks work. So for now if a project uses any of bun related stuff gradle builds will just fail. The only thing I can think of is providing "temporary" Windows support via WSL2. But it would have a few quirks:

My suggestion would be to make it as simple as possible: if before-mentioned feature flag is turned on then we will assume that user has WSL installed as well as bun (on the WSL). So BunSetup task would be skipped in that case.

Let me know what you think about it, I could do a follow up PR with such "experimental" support.

PS I'm a Windows user so it's feasible to develop it on my end, but I don't know how it will look in terms of testing on CI

deepy commented 10 months ago

We'll skip initial Windows support, WSL support might be useful on its own (in general for the plugin), but is a major change (code-wise) and can wait :-)

deepy commented 10 months ago

From an initial glance this looks good, I'm looking into the test failures and some of them are caused by tests that assume local binaries being available (which is ok, I'll fix that on CI later) There's some slight cleanup that needs to be done, but I can do that after merging

deepy commented 10 months ago

Okay there's some little-bit-more-than-slight cleanup that needs to be done as well, I'll work on that after merging but I'm gonna list the things here before extracting them to a new issue

deepy commented 10 months ago

Most of that is because the general test setup in gradle-node-plugin is unwieldy :-)