-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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(core): support bun as a package manager #22402
Changes from 33 commits
b4122c3
35c9744
98cc98e
a6484a1
050f768
c973c4c
b11d379
400f235
3660264
1c7299e
1a87629
5636c3a
13c9362
a9170cc
c4e3c11
7399bfa
fa39815
5097b67
4471261
8cff75d
9ac2764
c4d2c79
0155745
d58cec2
658e1c3
87f19d6
6bc6cce
3fc8669
d49f51f
a63dafa
b84fc55
daf364a
2ed0ea0
46c703f
6abfb0c
c1b2d6d
33e629d
1adf9d9
17ff9a8
782aeb0
931392c
ba6a5bc
3e07ddc
85a0eef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
#!/usr/bin/env sh | ||
|
||
pnpm check-documentation-map && | ||
pnpm check-lock-files && | ||
pnpm check-commit && | ||
pnpm documentation && | ||
pnpm pretty-quick --check | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ export function createApplicationFiles(host: Tree, options: NormalizedSchema) { | |
npm: 'package-lock.json', | ||
yarn: 'yarn.lock', | ||
pnpm: 'pnpm-lock.yaml', | ||
bun: 'bun.lockb', | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added bun.js lock file |
||
const packageManager = detectPackageManager(host.root); | ||
const packageLockFile = packageManagerLockFile[packageManager]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ export default function update(tree: Tree) { | |
npm: 'package-lock.json', | ||
yarn: 'yarn.lock', | ||
pnpm: 'pnpm-lock.yaml', | ||
bun: 'bun.lockb', | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added bun.js lock file |
||
|
||
for (const [name, config] of projects.entries()) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,7 +53,7 @@ export const parserConfiguration: Partial<yargs.ParserConfigurationOptions> = { | |
* from the `.argv` call, so the object and it's relative scripts can | ||
* le executed correctly. | ||
*/ | ||
export const commandsObject = yargs | ||
export const commandsObject: any = yargs | ||
.parserConfiguration(parserConfiguration) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type check for any There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert this, I'm not sure why it was added There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated for CI check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This again must be something unusual about your setup. There is no known general issue with this code on master, nor on mine or other's machines. Please revert this change and let's see what happens in CI There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either that or please link me to a CI run where this failed, I cannot see one |
||
.usage(chalk.bold('Smart Monorepos · Fast CI')) | ||
.demandCommand(1, '') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,8 +115,8 @@ export const yargsReleaseCommand: CommandModule< | |
); | ||
} | ||
const nxJson = readNxJson(); | ||
if (argv.groups?.length) { | ||
for (const group of argv.groups) { | ||
if ((argv.groups as string[] | string)?.length) { | ||
for (const group of argv.groups as string[] | string) { | ||
if (!nxJson.release?.groups?.[group]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type checks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are not type checks, they are the opposite. They are type assertions, where you are telling the compiler what the type is. Please revert I'm not sure why you are touching release command object in a PR regarding bun There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I just updated the error returned with the types given in the error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is for the green check.... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This again must be something unusual about your setup. There is no known general issue with this code on master, nor on mine or other's machines. Please revert this change and let's see what happens in CI There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either that or please link me to a CI run where this failed, I cannot see one |
||
throw new Error( | ||
`The specified release group "${group}" was not found in nx.json` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -534,7 +534,7 @@ function printAndFlushChanges(tree: Tree, isDryRun: boolean) { | |
isDryRun ? chalk.keyword('orange')(' [dry-run]') : '' | ||
}` | ||
); | ||
printDiff('', f.content?.toString() || ''); | ||
printDiff('', f.content?.toString() ?? ''); | ||
} else if (f.type === 'UPDATE') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. release version.ts is not relevant to a PR regarding bun, please revert |
||
console.error( | ||
`${chalk.white('UPDATE')} ${f.path}${ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,7 +39,7 @@ export interface NxArgs { | |
} | ||
|
||
export function createOverrides(__overrides_unparsed__: string[] = []) { | ||
let overrides = | ||
let overrides: any = | ||
yargsParser(__overrides_unparsed__, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typecheck There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this marked as resolved, it hasn't been reverted yet? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because it's required for the documentation command to run. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is for the green check! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried. It didn't work without them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above, please revert |
||
configuration: { | ||
'camel-case-expansion': false, | ||
|
@@ -121,7 +121,7 @@ export function splitArgsIntoNxArgsAndOverrides( | |
|
||
// Allow setting base and head via environment variables (lower priority then direct command arguments) | ||
if (!nxArgs.base && process.env.NX_BASE) { | ||
nxArgs.base = process.env.NX_BASE; | ||
nxArgs.base = process.env.NX_BASE ?? 'main'; | ||
if (options.printWarnings) { | ||
laynef marked this conversation as resolved.
Show resolved
Hide resolved
|
||
output.note({ | ||
title: `No explicit --base argument provided, but found environment variable NX_BASE so using its value as the affected base: ${output.bold( | ||
|
@@ -143,7 +143,7 @@ export function splitArgsIntoNxArgsAndOverrides( | |
|
||
if (!nxArgs.base) { | ||
nxArgs.base = | ||
nxJson.defaultBase ?? nxJson.affected?.defaultBase ?? 'main'; | ||
nxJson?.defaultBase ?? nxJson?.affected?.defaultBase ?? 'main'; | ||
|
||
laynef marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// No user-provided arguments to set the affected criteria, so inform the user of the defaults being used | ||
if ( | ||
|
@@ -290,7 +290,7 @@ function getUntrackedFiles(): string[] { | |
return parseGitOutput(`git ls-files --others --exclude-standard`); | ||
} | ||
|
||
function getMergeBase(base: string, head: string = 'HEAD') { | ||
function getMergeBase(base: string = 'main', head: string = 'HEAD') { | ||
try { | ||
laynef marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return execSync(`git merge-base "${base}" "${head}"`, { | ||
maxBuffer: TEN_MEGABYTES, | ||
|
@@ -314,7 +314,10 @@ function getMergeBase(base: string, head: string = 'HEAD') { | |
} | ||
} | ||
|
||
function getFilesUsingBaseAndHead(base: string, head: string): string[] { | ||
function getFilesUsingBaseAndHead( | ||
base: string = 'main', | ||
head: string | ||
): string[] { | ||
return parseGitOutput( | ||
laynef marked this conversation as resolved.
Show resolved
Hide resolved
|
||
`git diff --name-only --no-renames --relative "${base}" "${head}"` | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,65 +16,94 @@ describe('package-manager', () => { | |
packageManager: 'pnpm', | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated unit test |
||
}); | ||
jest.spyOn(fs, 'existsSync').mockImplementation((p) => { | ||
switch (p) { | ||
case 'pnpm-lock.yaml': | ||
return true; | ||
case 'yarn.lock': | ||
return false; | ||
case 'package-lock.json': | ||
return false; | ||
case 'bun.lockb': | ||
return false; | ||
default: | ||
return jest.requireActual('fs').existsSync(p); | ||
} | ||
}); | ||
const packageManager = detectPackageManager(); | ||
expect(packageManager).toEqual('pnpm'); | ||
}); | ||
|
||
it('should detect yarn package manager from yarn.lock', () => { | ||
jest.spyOn(configModule, 'readNxJson').mockReturnValueOnce({}); | ||
jest.spyOn(configModule, 'readNxJson').mockReturnValueOnce({ | ||
cli: { | ||
packageManager: 'yarn', | ||
}, | ||
}); | ||
jest.spyOn(fs, 'existsSync').mockImplementation((p) => { | ||
switch (p) { | ||
case 'yarn.lock': | ||
return true; | ||
case 'pnpm-lock.yaml': | ||
return false; | ||
case 'yarn.lock': | ||
return true; | ||
case 'package-lock.json': | ||
return false; | ||
case 'bun.lockb': | ||
return false; | ||
default: | ||
return jest.requireActual('fs').existsSync(p); | ||
} | ||
}); | ||
const packageManager = detectPackageManager(); | ||
expect(packageManager).toEqual('yarn'); | ||
expect(fs.existsSync).toHaveBeenNthCalledWith(1, 'yarn.lock'); | ||
}); | ||
|
||
it('should detect pnpm package manager from pnpm-lock.yaml', () => { | ||
jest.spyOn(configModule, 'readNxJson').mockReturnValueOnce({}); | ||
it('should detect package manager in nxJson', () => { | ||
jest.spyOn(configModule, 'readNxJson').mockReturnValueOnce({ | ||
cli: { | ||
packageManager: 'bun', | ||
}, | ||
}); | ||
jest.spyOn(fs, 'existsSync').mockImplementation((p) => { | ||
switch (p) { | ||
case 'pnpm-lock.yaml': | ||
return false; | ||
case 'yarn.lock': | ||
return false; | ||
case 'pnpm-lock.yaml': | ||
return true; | ||
case 'package-lock.json': | ||
return false; | ||
case 'bun.lockb': | ||
return true; | ||
default: | ||
return jest.requireActual('fs').existsSync(p); | ||
} | ||
}); | ||
const packageManager = detectPackageManager(); | ||
expect(packageManager).toEqual('pnpm'); | ||
expect(fs.existsSync).toHaveBeenCalledTimes(3); | ||
expect(packageManager).toEqual('bun'); | ||
}); | ||
|
||
it('should use npm package manager as default', () => { | ||
jest.spyOn(configModule, 'readNxJson').mockReturnValueOnce({}); | ||
jest.spyOn(configModule, 'readNxJson').mockReturnValueOnce({ | ||
cli: { | ||
packageManager: 'npm', | ||
}, | ||
}); | ||
jest.spyOn(fs, 'existsSync').mockImplementation((p) => { | ||
switch (p) { | ||
case 'yarn.lock': | ||
return false; | ||
case 'pnpm-lock.yaml': | ||
return false; | ||
case 'yarn.lock': | ||
return false; | ||
case 'package-lock.json': | ||
return true; | ||
case 'bun.lockb': | ||
return false; | ||
default: | ||
return jest.requireActual('fs').existsSync(p); | ||
} | ||
}); | ||
const packageManager = detectPackageManager(); | ||
expect(packageManager).toEqual('npm'); | ||
expect(fs.existsSync).toHaveBeenCalledTimes(5); | ||
}); | ||
}); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ import { workspaceRoot } from './workspace-root'; | |
|
||
const execAsync = promisify(exec); | ||
|
||
export type PackageManager = 'yarn' | 'pnpm' | 'npm'; | ||
export type PackageManager = 'yarn' | 'pnpm' | 'npm' | 'bun'; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added bun |
||
export interface PackageManagerCommands { | ||
preInstall?: string; | ||
|
@@ -29,19 +29,26 @@ export interface PackageManagerCommands { | |
run: (script: string, args: string) => string; | ||
} | ||
|
||
/** | ||
* Detects which package manager is used in the workspace based on the lock file. | ||
*/ | ||
function getPackageManager(dir: string = '') { | ||
return existsSync(join(dir, 'yarn.lock')) | ||
? 'yarn' | ||
: existsSync(join(dir, 'bun.lockb')) | ||
? 'bun' | ||
: existsSync(join(dir, 'pnpm-lock.yaml')) || | ||
existsSync(join(dir, 'pnpm-workspace.yaml')) | ||
? 'pnpm' | ||
: 'npm'; | ||
} | ||
|
||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added bun lock logic in util function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the bun lock file check should come first. According to https://bun.sh/docs/install/lockfile a feature of bun is that it will generate a yarn.lock file for users to more easily inspect. So the presence of a yarn.lock file could actually still mean that it is a bun repo (as long as there is a also a Therefore I think the simplest thing is to simply apply the check for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However please see my next comment about returning this logic to its original location |
||
* Detects which package manager is used in the workspace based on the lock file. | ||
*/ | ||
export function detectPackageManager(dir: string = ''): PackageManager { | ||
const nxJson = readNxJson(); | ||
return ( | ||
nxJson.cli?.packageManager ?? | ||
(existsSync(join(dir, 'yarn.lock')) | ||
? 'yarn' | ||
: existsSync(join(dir, 'pnpm-lock.yaml')) | ||
? 'pnpm' | ||
: 'npm') | ||
); | ||
return nxJson.cli?.packageManager ?? getPackageManager(dir); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Used util to call default lock file lookup There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert this so that the logic is back to being inline in this small utility function. There is no value in splitting this out into its own function when the additional logic is 2 lines and the other function is private and never reused (see the comments above for the exact logic to use and comment to add) |
||
|
||
/** | ||
|
@@ -142,6 +149,23 @@ export function getPackageManagerCommand( | |
list: 'npm ls', | ||
}; | ||
}, | ||
bun: () => { | ||
// TODO: Remove this | ||
process.env.npm_config_legacy_peer_deps ??= 'true'; | ||
laynef marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return { | ||
install: 'bun install', | ||
ciInstall: 'bun install --no-cache', | ||
updateLockFile: 'bun install --frozen-lockfile', | ||
add: 'bun install', | ||
addDev: 'bun install -D', | ||
rm: 'bun rm', | ||
exec: 'bun', | ||
dlx: 'bunx', | ||
run: (script: string, args: string) => `bun run ${script} -- ${args}`, | ||
list: 'bun pm ls', | ||
}; | ||
}, | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added bun commands |
||
|
||
return commands[packageManager](); | ||
|
@@ -230,7 +254,12 @@ export function copyPackageManagerConfigurationFiles( | |
root: string, | ||
destination: string | ||
) { | ||
for (const packageManagerConfigFile of ['.npmrc', '.yarnrc', '.yarnrc.yml']) { | ||
for (const packageManagerConfigFile of [ | ||
'.npmrc', | ||
'.yarnrc', | ||
'.yarnrc.yml', | ||
'bunfig.toml', | ||
]) { | ||
// f is an absolute path, including the {workspaceRoot}. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added bun config |
||
const f = findFileInPackageJsonDirectory(packageManagerConfigFile, root); | ||
if (f) { | ||
|
@@ -243,6 +272,10 @@ export function copyPackageManagerConfigurationFiles( | |
copyFileSync(f, destinationPath); | ||
break; | ||
} | ||
case 'bunfig.toml': { | ||
copyFileSync(f, destinationPath); | ||
break; | ||
} | ||
case '.yarnrc': { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added bun toml |
||
const updated = modifyYarnRcToFitNewDirectory(readFileIfExisting(f)); | ||
writeFileSync(destinationPath, updated); | ||
|
@@ -343,7 +376,7 @@ export async function packageRegistryView( | |
args: string | ||
): Promise<string> { | ||
let pm = detectPackageManager(); | ||
if (pm === 'yarn') { | ||
if (pm === 'yarn' || pm === 'bun') { | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use case needs to follow yarn here. npm view does not exist for bun |
||
* yarn has `yarn info` but it behaves differently than (p)npm, | ||
* which makes it's usage unreliable | ||
|
@@ -363,7 +396,7 @@ export async function packageRegistryPack( | |
version: string | ||
): Promise<{ tarballPath: string }> { | ||
let pm = detectPackageManager(); | ||
if (pm === 'yarn') { | ||
if (pm === 'yarn' || pm === 'bun') { | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use case needs to follow yarn here. npm pack does not exist for bun |
||
* `(p)npm pack` will download a tarball of the specified version, | ||
* whereas `yarn` pack creates a tarball of the active workspace, so it | ||
|
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.
Added check instead of generate to remove the revert issue
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.
Please revert this. It must be something specific about your setup/environment, other users do not reorder properties when they run this. I just verified on my machine
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.
Can't or else it generates files.
Please expect this as a check instead a write command on the git hook.
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.
It's a manditory change to revert these files that are generated from this hook.
I am checking them instead of generating them. Generating them can be done from the command:
pnpm run documentation
Just not on the git hook
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.
That's what I'm saying though, it doesn't for anyone else (I pulled down this branch and ran it myself), each PR that goes into this repo is running the same commands. There is something unique about it running on your machine. We can figure that our separately, but for this PR we won't be making this change
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.
You can push with
--no-verify
after you make the change to make sure your unique issue doesn't impact the filesThere 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.
git push --no-verify
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.
Gotcha. Sorry misunderstood. I will remove this change.