feat: support bun package manager and bun workspaces#106
feat: support bun package manager and bun workspaces#1069romise merged 15 commits intonpmx-dev:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request adds support for Bun as a package manager alongside npm, pnpm, and yarn. It introduces workspace catalog extraction functionality through the extended 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/language-server/src/merge-resolved-dependencies.ts (1)
7-10: Return a fresh array in every non-undefinedpath for consistent immutability.Current behaviour returns a copy when both inputs exist, but returns the original reference when only one exists. A consistent copy-on-return pattern is safer.
Proposed refactor
export function mergeResolvedDependencies( manifestDependencies?: DependencyInfo[], workspaceDependencies?: DependencyInfo[], ): DependencyInfo[] | undefined { - if (manifestDependencies && workspaceDependencies) - return [...manifestDependencies, ...workspaceDependencies] - - return manifestDependencies ?? workspaceDependencies + if (!manifestDependencies && !workspaceDependencies) + return undefined + + return [ + ...(manifestDependencies ?? []), + ...(workspaceDependencies ?? []), + ] }packages/language-core/src/extractors/json.test.ts (1)
46-69: Add dependency assertions for the nestedworkspacescatalog case.This test currently validates only
catalogs. Assertingdependenciestoo would better guard against regressions in flattening logic.Proposed test extension
it('extracts catalogs nested inside the workspaces object', () => { const info = extractor.getWorkspaceCatalogInfo(`{ "workspaces": { "packages": ["packages/*"], "catalog": { "react": "^19.0.0" }, "catalogs": { "test": { "vitest": "^4.0.0" } } } }`) expect(info?.catalogs).toEqual({ default: { react: '^19.0.0', }, test: { vitest: '^4.0.0', }, }) + expect(info?.dependencies.map(({ rawName, rawSpec, categoryName }) => ({ + rawName, + rawSpec, + categoryName, + }))).toEqual([ + { + rawName: 'react', + rawSpec: '^19.0.0', + categoryName: '', + }, + { + rawName: 'vitest', + rawSpec: '^4.0.0', + categoryName: 'test', + }, + ]) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccbfa554-719f-48be-b1b2-85707c3dfbaf
📒 Files selected for processing (14)
README.mdextensions/vscode/README.mdpackages/language-core/src/extractors/json.test.tspackages/language-core/src/extractors/json.tspackages/language-core/src/workspace.test.tspackages/language-core/src/workspace.tspackages/language-server/src/merge-resolved-dependencies.tspackages/language-server/src/workspace.test.tspackages/language-server/src/workspace.tsplayground/bun/.vscode/settings.jsonplayground/bun/package.jsonplayground/bun/packages/app/package.jsonplayground/bun/packages/utils/package.jsonplayground/playground.code-workspace
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/language-core/src/workspace.test.ts (1)
39-39: Consider renaming this test for accuracy.The current title says “loads”, but the assertions verify the missing-file path returning
undefined. A tighter name will reduce ambiguity when failures are scanned.✏️ Suggested rename
- it('still loads workspace catalogs for pnpm workspaces', async () => { + it('returns undefined when the pnpm workspace file is missing', async () => {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 640385d0-80de-4858-b674-28c5899ef6a9
📒 Files selected for processing (3)
packages/language-core/src/workspace.test.tspackages/language-core/src/workspace.tspackages/language-server/src/workspace.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/language-server/src/workspace.ts
- packages/language-core/src/workspace.ts
9romise
left a comment
9romise
left a comment
There was a problem hiding this comment.
Thank you for your awesome work!
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/language-server/src/workspace.ts (2)
163-178: Optional: parallelise the two loads for the Bun root manifest.For the Bun case where
uri.path === ctx.workspaceFilePath,loadPackageManifestInfoandloadWorkspaceFileInfoare independent and could run concurrently on the initial (un-cached) call:♻️ Proposed change
- const manifestDependencies = (await ctx.loadPackageManifestInfo(uri.path))?.dependencies - if (ctx.packageManager !== 'bun' || ctx.workspaceFilePath !== uri.path) - return manifestDependencies - - const workspaceDependencies = (await ctx.loadWorkspaceFileInfo(uri.path))?.dependencies - return mergeResolvedDependencies(manifestDependencies, workspaceDependencies) + if (ctx.packageManager !== 'bun' || ctx.workspaceFilePath !== uri.path) + return (await ctx.loadPackageManifestInfo(uri.path))?.dependencies + + const [manifestInfo, workspaceInfo] = await Promise.all([ + ctx.loadPackageManifestInfo(uri.path), + ctx.loadWorkspaceFileInfo(uri.path), + ]) + return mergeResolvedDependencies(manifestInfo?.dependencies, workspaceInfo?.dependencies)
40-49: The client-side handler already correctly supports the new URI format; consider removing the unusedrootPathparameter from theWorkspaceAdapterinterface.The payload change from
rootPathtofolderUri.toString()is already handled correctly on the client side. The handler inextensions/vscode/src/request.ts(Line 18) callsUri.parse(params.uri), which correctly expects and processes a full URI string rather than a bare path. No client-side changes are needed.However, the
WorkspaceAdapterinterface inpackages/language-core/src/workspace.ts(Line 32) still declaresdetectPackageManager: (rootPath: string) => Promise<PackageManager>, and the call site at Line 105 passesthis.rootPathas an argument. The language-server implementation (Line 40 ofpackages/language-server/src/workspace.ts) omits this parameter entirely and never uses it. Remove the unused parameter from both the interface signature and the call site to clarify the actual contract.packages/language-core/src/workspace.ts (1)
11-12: Finish the migration frompathetopath-browserify.The previous review established that
pathe.joinnormalises away the leading/of Windows URI paths (/D:/repo/...→D:/repo/...), breaking equality withURI.path. Althoughjoinhas been migrated,dirnameon line 12 is still imported frompathe. It is used twice infindNearestPackageManifestPath(lines 178 and 189) where results are compared againstthis.rootPathviadir.startsWith(\${this.rootPath}/`). Ifpathe.dirname` applies the same normalisation, the walk will silently never match on Windows.Consolidate both onto
path-browserifyto maintain consistent path semantics withURI.paththroughout the file.♻️ Proposed changes
-import path from 'path-browserify' -import { dirname } from 'pathe' +import path from 'path-browserify'- let dir = dirname(packageManifestPath) + let dir = path.posix.dirname(packageManifestPath)- const parent = dirname(dir) + const parent = path.posix.dirname(dir)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ed8f9b8-f770-4cc9-93e6-342f695427ba
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
packages/language-core/package.jsonpackages/language-core/src/workspace.tspackages/language-core/tsdown.config.tspackages/language-server/src/workspace.test.tspackages/language-server/src/workspace.tspnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (3)
- packages/language-core/tsdown.config.ts
- pnpm-workspace.yaml
- packages/language-server/src/workspace.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38edb352-b734-449e-96e4-10104ac89e78
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
package.jsonpackages/language-core/package.jsonpackages/language-core/src/extractors/index.tspackages/language-core/src/utils/file.tspackages/language-core/src/workspace.tspackages/language-core/tsdown.config.tspackages/language-server/src/workspace.tspnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (3)
- packages/language-core/src/extractors/index.ts
- package.json
- packages/language-core/tsdown.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- pnpm-workspace.yaml
- packages/language-core/package.json
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83d6ac82-ab77-4d1c-a140-32e6b1d23dde
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
package.jsonpackages/language-core/package.jsonpackages/language-core/src/extractors/index.tspackages/language-core/src/utils/file.tspackages/language-core/src/workspace.tspackages/language-core/tsdown.config.tspackages/language-server/src/workspace.tspnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (6)
- package.json
- packages/language-core/tsdown.config.ts
- packages/language-core/src/extractors/index.ts
- pnpm-workspace.yaml
- packages/language-core/package.json
- packages/language-core/src/workspace.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/language-core/src/utils/file.ts
3 participants
related #102
Changes:
catalog,catalogs,workspaces.catalog, etc in Bun workspacespackage.jsonmanifest and catalog entries for hover/diagnosticsplayground/bunto a real Bun workspace example