feat: support bun package manager and bun workspaces#106
feat: support bun package manager and bun workspaces#106RYGRIT wants to merge 8 commits intonpmx-dev:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds Bun support across the workspace and extraction layers: expands Possibly related PRs
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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!
|
|
||
| const TRAILING_SLASHES_RE = /\/+$/ | ||
|
|
||
| function joinUriPath(dir: string, basename: string): string { |
There was a problem hiding this comment.
Why do we need a custom join instead of the pathe one? Are there some specific reasons?
There was a problem hiding this comment.
The custom processing here is because on Windows, URI.path is /d:/repo/package.json, and if pathe.join() is used, it will be normalized to d:/repo/package.json, causing the string comparison with URI.path to fail.
// demo.ts
import { join } from 'pathe'
import { URI } from 'vscode-uri'
const uri = URI.file('D:/repo/playground/bun/package.json')
console.log('uri.toString():', uri.toString())
console.log('uri.path:', uri.path)
const rootPath = URI.file('D:/repo/playground/bun').path
console.log('rootPath:', rootPath)
console.log('pathe.join:', join(rootPath, 'package.json'))
function joinUriPath(dir: string, basename: string): string {
return `${dir.replace(/\/+$/, '')}/${basename}`
}
console.log('joinUriPath:', joinUriPath(rootPath, 'package.json'))
console.log('pathe matches uri.path:', join(rootPath, 'package.json') === uri.path)
console.log('custom matches uri.path:', joinUriPath(rootPath, 'package.json') === uri.path)uri.toString(): file:///d%3A/repo/playground/bun/package.json
uri.path: /D:/repo/playground/bun/package.json
rootPath: /D:/repo/playground/bun
pathe.join: D:/repo/playground/bun/package.json
joinUriPath: /D:/repo/playground/bun/package.json
pathe matches uri.path: false
custom matches uri.path: true
There was a problem hiding this comment.
We might need to replace pathe with path-browserify. I've tried it locally and it works fine. Besides, it’s already a dependency of Volar, so this won’t increase the bundle size.
There was a problem hiding this comment.
Sounds good. I’ll test path-browserify locally against the Windows URI path case (/d:/...) and push a follow-up commit if it preserves the expected behavior.
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
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