ariakit icon indicating copy to clipboard operation
ariakit copied to clipboard

Experiment: use Bun as package manager and script runner

Open DaniGuardiola opened this issue 1 year ago • 26 comments

Just experimenting with Bun to see what's possible. This removes lerna as it's no longer necessary.

Tested:

  • all affected scripts (build, test, test-browser, clean, coverage, generate-images, etc)
  • most untouched scripts, but run with bun instead of npm (e.g. bun lint-fix
  • main ci tasks
  • plus ci
  • husky

Untested:

  • changeset script
  • reactnext ci
  • release ci
  • test-vo ci
  • vercel build
  • codesandbox?

To do still:

  • [x] package patching
  • [x] @wordpress/components thing
  • [ ] complete testing
  • [ ] update docs (contribution, etc)
  • [ ] double-check everything

Future ideas:

  • Replace vitest with bun test

DaniGuardiola avatar Jan 08 '25 22:01 DaniGuardiola

⚠️ No Changeset found

Latest commit: a5a0914d12cfb50e56d9f7262c554546f98b1391

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Jan 08 '25 22:01 changeset-bot[bot]

Regarding Vercel failures: https://www.vercel-status.com

diegohaz avatar Jan 08 '25 23:01 diegohaz

@diegohaz nice timing lol, thanks for sharing. Almost done here, just need to tick off the last few items and test the remaining stuff. Any help (especially with testing) and/or thoughts about this are welcome!

DaniGuardiola avatar Jan 08 '25 23:01 DaniGuardiola

The last time I tested bun test, they didn't support JSDOM, and HappyDOM wasn't as feature complete. I'm not sure how it is today.

Anyway, I'm pretty happy with Vitest. Tests will still be a bit slow because we add fake timeouts to simulate real user interaction. Unless bun can parallelize more, I don't expect a big difference.

diegohaz avatar Jan 09 '25 00:01 diegohaz

@diegohaz thanks for the insight! To be clear, I'm not aiming for bun test in this PR, it was just an idea, but that's good to know. I think bun as pm and script runner is worth it though.

DaniGuardiola avatar Jan 09 '25 01:01 DaniGuardiola

I think this file needs updating too? https://github.com/ariakit/ariakit/blob/ff2509c7d7ce75a6e45a56f1cc61302a5dc6632b/vercel.json#L1-L7

diegohaz avatar Jan 09 '25 02:01 diegohaz

@diegohaz updated vercel.json. I can't access the build logs so let me know if it worked or if there are any errors. Same with codesandbox.

Question: is the changeset patch still needed? If so, I'll migrate it to bun's patching system. If not, we should be able to just remove it.

Separately, I could use a bit of help testing some of the remaining stuff. I'm gonna test changeset locally, but the remaining CI stuff and vercel/codesandbox are a bit trickier as a contributor, happy to do the work if you tell me how you prefer to test it though.

DaniGuardiola avatar Jan 09 '25 15:01 DaniGuardiola

@diegohaz updated vercel.json. I can't access the build logs so let me know if it worked or if there are any errors. Same with codesandbox.

There's a warning regarding the @wordpress/components override:

warn: Bun currently does not support nested "overrides"
   at /vercel/path0/package.json:69:30

This override is necessary because we have tests for @wordpress/components and need to use the local version of @ariakit/react instead of the npm version that WordPress relies on.

Also, there's an error at the end of the website build, but since the site appears to build correctly, I'm unsure what it means:

website build: Exited with code 0

Question: is the changeset patch still needed? If so, I'll migrate it to bun's patching system. If not, we should be able to just remove it.

This issue is still open, so I believe the patch is still necessary: https://github.com/changesets/changesets/issues/995

Regarding CodeSandbox, I think there might be some settings that need adjusting for Bun in this file:

https://github.com/ariakit/ariakit/blob/1a279070efbb83fda9f74cb8c5677832e84750a0/.codesandbox/ci.json#L1-L6

CodeSandbox CI hasn't been very useful. I need to switch to https://pkg.pr.new at some point.

diegohaz avatar Jan 09 '25 17:01 diegohaz

Also, leaving package-lock.json unchanged for now until this PR is merged might help avoid conflicts. Otherwise, there will be a conflict with main every time the Renovate bot merges a PR.

diegohaz avatar Jan 09 '25 17:01 diegohaz

Re:override, wdy think about this? https://github.com/oven-sh/bun/issues/6608#issuecomment-2162028520

TL;DR: instead of nested just apply the override globally. AFAIK, this is the only case where it'll be pulled as a module, the monorepo setup and everything else should be unaffected, right?

DaniGuardiola avatar Jan 10 '25 16:01 DaniGuardiola

A global override might work too. If we add more packages that depend on Ariakit, we'll probably want to do the same for them.

diegohaz avatar Jan 10 '25 16:01 diegohaz

Also, there's an error at the end of the website build, but since the site appears to build correctly, I'm unsure what it means

Code 0 means success, there's no error. It's just a log by Bun, probably because of the use of --filter to target the website worspace. A "meta-log" from the task runner, essentially.

DaniGuardiola avatar Jan 10 '25 16:01 DaniGuardiola

Also, there's an error at the end of the website build, but since the site appears to build correctly, I'm unsure what it means

Code 0 means success, there's no error. It's just a log by Bun, probably because of the use of --filter to target the website worspace. A "meta-log" from the task runner, essentially.

Yes, it was just strange that it was reported as an error:

Screenshot of Vercel logs with error saying, "website build: Exited with code 0"

diegohaz avatar Jan 10 '25 16:01 diegohaz

Right, I guess it is being outputed to stderr. Weird thing is I don't see it locally. Well, if it works, I guess it's fine.

Re:override, can you think of a small change I can make in the source to produce a test error for the wp package? The goal is to have a way to verify that it's being run with the local version and not one from npm.

I have a theory that it might not be necessary at all, because it should already be using the workspace: protocol and ~the npm version isn't even installed~ Nvm just realized it should be getting installed along with wp components. Either way, it'll be helpful to have a way to verify it.

DaniGuardiola avatar Jan 10 '25 16:01 DaniGuardiola

Doesn't look like the Codesandbox CI supports Bun at all (inferred from what I've seen in the docs, there's no way to install it). Might be better to just remove it. The stackblitz alternative depends on your own CI so it should be much easier to use. Maybe I'll try to get that done in a separate PR before merging this one.

DaniGuardiola avatar Jan 10 '25 16:01 DaniGuardiola

Huh, did you delete your last comment with the trick to make tests fail? @diegohaz

I reloaded the issue and it's gone now.

DaniGuardiola avatar Jan 10 '25 17:01 DaniGuardiola

Good news, seems like the override is entirely unnecessary. I used your trick to test. I'm gonna wait for CI to pass and then push a commit with the trick to verify that CI behavior is the same. But I think we're in the clear with that.

Remaining:

  • patch-package thing
  • CodeSandbox (maybe we just remove it here?)
  • test the untested stuff

DaniGuardiola avatar Jan 10 '25 17:01 DaniGuardiola

Huh, did you delete your last comment with the trick to make tests fail? @diegohaz

I reloaded the issue and it's gone now.

Sorry, that information wasn't accurate.

If @wordpress/components relies on the same version as the local package, npm seems to automatically resolve it to the local package. The issue arises when there's a version mismatch. In that case, the remote npm package will be installed inside node_modules/@wordpress/components/node_modules.

This should make npm run test-chrome wordpress fail some tests. But I'm not sure how to manually trigger this while making overrides work simultaneously to verify if overrides are working.

If this is correct, it will become a problem if Ariakit 0.5.0 is released and @wordpress/components still depends on ^0.4.15.

diegohaz avatar Jan 10 '25 17:01 diegohaz

Could it be replicated if I manually set the local version to 0.5 temporarily?

DaniGuardiola avatar Jan 10 '25 17:01 DaniGuardiola

I think it's easier if you install a version of @wordpress/components that depend on ariakit@^0.3 instead like @wordpress/[email protected]. I couldn't reproduce it consistently, though.

diegohaz avatar Jan 10 '25 17:01 diegohaz

FWIW, I tweaked the package.json and ran bun i:

image

The trick still causes it to fail. Not sure if that's enough to verify but it seems like a good sign.

DaniGuardiola avatar Jan 10 '25 17:01 DaniGuardiola

I'll leave figuring this out to you if you don't mind. I'm not aware of all the constrains here.

DaniGuardiola avatar Jan 10 '25 17:01 DaniGuardiola

This in the bun.lock file is a bit sus:

image

DaniGuardiola avatar Jan 10 '25 17:01 DaniGuardiola

Here's how you can check if overrides is working using npm:

  1. Install @wordpress/[email protected] (which depends on @ariakit/react@^0.3):

    npm i @wordpress/[email protected]
    
  2. Verify whether the nested dependency has been overridden:

    test -e node_modules/@wordpress/components/node_modules/@ariakit/react/package.json && echo "duplicate dependency" || echo "hoisted dependency"
    

    If it outputs hoisted dependency, the overrides are working.

You can remove the overrides option and repeat the steps. It should output duplicate dependency.

diegohaz avatar Jan 11 '25 06:01 diegohaz

~Seems like patch-package works as it is. I've also read that the Bun patching implementation is a bit buggy still so we can just migrate in the future.~

Update: migrated anyway, seems to work fine.

DaniGuardiola avatar Feb 08 '25 20:02 DaniGuardiola

Blocked by https://github.com/oven-sh/bun/issues/17192

DaniGuardiola avatar Feb 09 '25 19:02 DaniGuardiola