Skip to content
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

Closed
wants to merge 44 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
b4122c3
feat(nx-dev): Added bun support
laynef Mar 16, 2024
35c9744
Merge branch 'master' into bun.js
laynef Mar 16, 2024
98cc98e
feat(nx-dev): Updated bun.js support
laynef Mar 16, 2024
a6484a1
fix(js): fixed typing
laynef Mar 16, 2024
050f768
fix(js): added docs
laynef Mar 16, 2024
c973c4c
fix(js): docs deleted
laynef Mar 16, 2024
b11d379
fix(js): fixed documentation
Mar 16, 2024
400f235
fix(js): Added bun.js support
laynef Mar 19, 2024
3660264
Merge branch 'master' into bun-support
laynef Mar 19, 2024
1c7299e
Merge branch 'master' into bun-support
laynef Mar 19, 2024
1a87629
fix(js): Added package json updates
laynef Mar 20, 2024
5636c3a
Merge branch 'bun-support' of github.com:laynef/nx into bun-support
laynef Mar 20, 2024
13c9362
Merge branch 'master' into bun-support
laynef Mar 20, 2024
a9170cc
fix(js): added package.json
laynef Mar 20, 2024
c4e3c11
Merge branch 'bun-support' of github.com:laynef/nx into bun-support
laynef Mar 20, 2024
7399bfa
fix(js): ran formatter
laynef Mar 20, 2024
fa39815
fix(js): ran re-ci
laynef Mar 20, 2024
5097b67
fix(js): updated readme
laynef Mar 20, 2024
4471261
fix(js): empty commit for ci
laynef Mar 20, 2024
8cff75d
fix(js): ran formatter
laynef Mar 20, 2024
9ac2764
Merge branch 'master' into bun-support
laynef Mar 20, 2024
c4d2c79
fix(js): Reverted docs and generated files
laynef Mar 22, 2024
0155745
fix(js): reverted nx-dev
laynef Mar 22, 2024
d58cec2
Merge branch 'bun-support' of github.com:laynef/nx into bun-support
laynef Mar 22, 2024
658e1c3
fix(js): fixing docs
laynef Mar 22, 2024
87f19d6
fix(js): Updated documentation
laynef Mar 22, 2024
6bc6cce
fix(js): reverted files
laynef Mar 22, 2024
3fc8669
Merge branch 'master' of github.com:nrwl/nx into bun-support
laynef Mar 22, 2024
d49f51f
fix(js): revert changes
laynef Mar 22, 2024
a63dafa
fix(js): re-run ci
laynef Mar 22, 2024
b84fc55
fix(js): Updated
laynef Mar 22, 2024
daf364a
fix(js): re-running command
laynef Mar 22, 2024
2ed0ea0
fix(js): removed snapshot
laynef Mar 22, 2024
46c703f
fix(js): updated tests
laynef Mar 22, 2024
6abfb0c
fix(js): reverted command line utils
laynef Mar 22, 2024
c1b2d6d
fix(js): reverted changes
laynef Mar 22, 2024
33e629d
fix(js): updated bun
laynef Mar 22, 2024
1adf9d9
fix(js): reverted more
laynef Mar 22, 2024
17ff9a8
fix(js): ran documentation
laynef Mar 22, 2024
782aeb0
fix(js): ran documentation
laynef Mar 22, 2024
931392c
fix(js): no-verify
laynef Mar 22, 2024
ba6a5bc
fix(js): made retime generator a devDep
laynef Mar 22, 2024
3e07ddc
Merge branch 'master' into bun-support
laynef Mar 22, 2024
85a0eef
fix(js): updated devDep to dep for CI
laynef Mar 22, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/generated/devkit/PackageManager.md
@@ -1,3 +1,3 @@
# Type alias: PackageManager

Ƭ **PackageManager**: `"yarn"` \| `"pnpm"` \| `"npm"`
Ƭ **PackageManager**: `"yarn"` \| `"pnpm"` \| `"npm"` \| `"bun"`
3 changes: 1 addition & 2 deletions package.json
Expand Up @@ -253,7 +253,6 @@
"react-refresh": "^0.10.0",
"react-router-dom": "^6.21.2",
"react-textarea-autosize": "^8.5.3",
"regenerator-runtime": "0.13.7",
JamesHenry marked this conversation as resolved.
Show resolved Hide resolved
"resolve.exports": "1.1.0",
"rollup": "^2.56.2",
"rollup-plugin-copy": "^3.4.0",
Expand Down Expand Up @@ -373,4 +372,4 @@
"documentation"
]
}
}
}
Expand Up @@ -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',
};
Copy link
Author

Choose a reason for hiding this comment

The 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];
Expand Down
Expand Up @@ -19,6 +19,7 @@ export default function update(tree: Tree) {
npm: 'package-lock.json',
yarn: 'yarn.lock',
pnpm: 'pnpm-lock.yaml',
bun: 'bun.lockb',
};
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added bun.js lock file


for (const [name, config] of projects.entries()) {
Expand Down
2 changes: 1 addition & 1 deletion packages/nx/src/command-line/nx-commands.ts
Expand Up @@ -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)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type check for any

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this, I'm not sure why it was added

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated for CI check pnpm run documentation

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

Choose a reason for hiding this comment

The 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, '')
Expand Down
4 changes: 2 additions & 2 deletions packages/nx/src/command-line/release/command-object.ts
Expand Up @@ -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]) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type checks

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Author

@laynef laynef Mar 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pnpm run documentation does not run without it.

I just updated the error returned with the types given in the error.

Copy link
Author

@laynef laynef Mar 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the green check....
I tried reverting these files.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

Choose a reason for hiding this comment

The 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`
Expand Down
2 changes: 1 addition & 1 deletion packages/nx/src/command-line/release/publish.ts
Expand Up @@ -136,7 +136,7 @@ async function runPublishOnProjects(
(projectName) => projectGraph.nodes[projectName]
);

const overrides = createOverrides(args.__overrides_unparsed__);
const overrides: any = createOverrides(args.__overrides_unparsed__);
laynef marked this conversation as resolved.
Show resolved Hide resolved

if (args.registry) {
overrides.registry = args.registry;
Expand Down
2 changes: 1 addition & 1 deletion packages/nx/src/utils/command-line-utils.ts
Expand Up @@ -39,7 +39,7 @@ export interface NxArgs {
}

export function createOverrides(__overrides_unparsed__: string[] = []) {
let overrides =
let overrides: any =
yargsParser(__overrides_unparsed__, {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typecheck

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this marked as resolved, it hasn't been reverted yet?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's required for the documentation command to run.
The CI requires the documentation command to work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the green check!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried. It didn't work without them.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above, please revert

configuration: {
'camel-case-expansion': false,
Expand Down
57 changes: 43 additions & 14 deletions packages/nx/src/utils/package-manager.spec.ts
Expand Up @@ -16,65 +16,94 @@ describe('package-manager', () => {
packageManager: 'pnpm',
},
Copy link
Author

Choose a reason for hiding this comment

The 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);
});
});

Expand Down
55 changes: 42 additions & 13 deletions packages/nx/src/utils/package-manager.ts
Expand Up @@ -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';

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added bun

export interface PackageManagerCommands {
preInstall?: string;
Expand All @@ -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';
}

/**
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added bun lock logic in util function

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 bun.lockb present).

Therefore I think the simplest thing is to simply apply the check for existsSync(join(dir, 'bun.lockb')) first and return bun. Please also add a comment in the code with this same explanation as to why we check for bun first

Copy link
Collaborator

Choose a reason for hiding this comment

The 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);
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used util to call default lock file lookup

Copy link
Collaborator

Choose a reason for hiding this comment

The 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)


/**
Expand Down Expand Up @@ -126,7 +133,6 @@ export function getPackageManagerCommand(
};
},
npm: () => {
// TODO: Remove this
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this, I did not ask you to remove the existing comment. I asked you to remove the copy of this comment you added fresh in your PR

process.env.npm_config_legacy_peer_deps ??= 'true';

return {
Expand All @@ -142,6 +148,20 @@ export function getPackageManagerCommand(
list: 'npm ls',
};
},
bun: () => {
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',
};
},
};
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added bun commands


return commands[packageManager]();
Expand Down Expand Up @@ -230,7 +250,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}.
Copy link
Author

@laynef laynef Mar 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added bun config

const f = findFileInPackageJsonDirectory(packageManagerConfigFile, root);
if (f) {
Expand All @@ -243,6 +268,10 @@ export function copyPackageManagerConfigurationFiles(
copyFileSync(f, destinationPath);
break;
}
case 'bunfig.toml': {
copyFileSync(f, destinationPath);
break;
}
case '.yarnrc': {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added bun toml

const updated = modifyYarnRcToFitNewDirectory(readFileIfExisting(f));
writeFileSync(destinationPath, updated);
Expand Down Expand Up @@ -343,7 +372,7 @@ export async function packageRegistryView(
args: string
): Promise<string> {
let pm = detectPackageManager();
if (pm === 'yarn') {
if (pm === 'yarn' || pm === 'bun') {
/**
Copy link
Author

Choose a reason for hiding this comment

The 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
Expand All @@ -363,7 +392,7 @@ export async function packageRegistryPack(
version: string
): Promise<{ tarballPath: string }> {
let pm = detectPackageManager();
if (pm === 'yarn') {
if (pm === 'yarn' || pm === 'bun') {
/**
Copy link
Author

Choose a reason for hiding this comment

The 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
Expand Down