fix(core): allow text lockfile for bun #29423
Conversation
ludicast
requested review from
a team,
meeroslav,
vsavkin,
mandarini,
FrozenPandaz,
AgentEnder,
jaysoo and
JamesHenry
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
View your CI Pipeline Execution ↗ for commit a594c26.
☁️ Nx Cloud last updated this comment at |
|
cc @Jordan-Hall |
e2e/utils/get-env-info.ts
Outdated
| return existsSync(join(dir, 'bun.lockb')) | ||
| ? 'bun' | ||
| : existsSync(join(dir, 'yarn.lock')) | ||
| return existsSync(join(dir, 'yarn.lock')) | ||
| ? 'yarn' |
There was a problem hiding this comment.
made yarn the first lockfile just to make the bun/pnpm indentation identical for multiple lockfiles
There was a problem hiding this comment.
e bun/pnpm indentation identical for multiple lockfiles
Doesnt bun current support to yarn lock still, This is why bun came frist
There was a problem hiding this comment.
right, great point, will reverse
|
@ludicast This is a good start but personally I would change the interface. We need to keep bun lock first because bun still goes to yarn with additional parm. I think its best to include a parser and writer as part of this PR. I did some work on this in preparation a while ago. Feel free to take a look at interface changes It also a branch from #27820 to add bun pack @ludicast can you add pack support too. |
|
Added the PR back for pack as their no breaking change. #29426 I should of never pulled the PR tbf |
thanks for info - changed the ordering back
we could but was thinking for that as a follow-on. I believe the format is jsonc (which isn't compatible with json.parse in vanilla js) so might want to avoid dealing with that in this PR (though now thinking that nx does parse a similar style in schemas anyways). esp the part of writing that lockfile (since .lock is a few days old and might surprise folks).
thanks @Jordan-Hall - gave it a look update - guess I prob can use the json-c parser if installed then nx/packages/nx/src/utils/json.ts Line 1 in ba1641d |
ludicast
changed the title
|
@Jordan-Hall - if by parser you mean writing something in the vein of https://github.com/nrwl/nx/blob/master/packages/nx/src/plugins/js/lock-file/yarn-parser.ts it def would need to be a follow-on (it's enough code to get caught up in PR purgatory) |
|
Yes i was because as you can see from the PR i did their loads of work needs to be done. I dont think just adding new vars is the right idea. If not can you at least do the grunt work without that section |
|
Hey @Jordan-Hall now that bun 1.2 has released text lock file by default, are there any workarounds until the required work is completed? |
|
nx now doesn't work with the default bun settings. Has this PR gone stale or are we working towards getting it merged? |
Whats the current issue. this PR directly wont resolve anything if im not mistaken |
|
@Jordan-Hall I believe they are referring to Bun 1.2 and onwards defaulting to the new text based lockfile |
|
@ludicast Thanks a lot for this! |
| @@ -182,7 +185,7 @@ function getLockFilePath(packageManager: PackageManager): string { | |||
| return NPM_LOCK_PATH; | |||
| } | |||
| if (packageManager === 'bun') { | |||
| return BUN_LOCK_PATH; | |||
| return BUN_LOCK_PATH; // defaulting to binary lockfile | |||
There was a problem hiding this comment.
I feel like we should be checking the Bun version and if >= 1.2 this should be defaulting to the text based one?
|
@ludicast You will please need to ensure CI is green, right now you have invalid syntax in this PR and the nx package cannot be built |
Even so their supporting both and I think the interface is no longer fit for purpose having single lock files. |
JamesHenry
changed the title
|
Just to let you know, I'm personally excited about |
|
@Jordan-Hall I understand where you are coming from, but (A) other package managers have had stable lock file names for many years (B) it seems unlikely would go through this churn again any time soon, so implementing some more complex handling just to make it feel like a cleaner implementation is probably overkill at this point. If something similar happened again we could definitely consider a broader refactor. @ludicast Thanks again for this, to ensure it gets merged as soon as possible I am going to fold the changes into the linked PR |
|
@Jordan-Hall the issue with the current defaults is I run into this bug: #29429. I have no binary lockfile, so I get the error
|
Current Behavior
Nx does not respect a
bun.lock, which is the new lockfile for bun (currently available and to be the default in bun1.2)The text-lockfile will allow dependabot to run, as well as prevent the merge conflicts caused with a binary lockfile.
Expected Behavior
Nx will respect a
bun.lockNote that we are still defaulting to the
bun.lockbstyle here for generators, this is just to allow repos containingbun.lock. Afterbun.lockbecomes the default there should be a second PR to make it the default for generators.Related Issue(s)
Fixes #29422