diff --git a/docs/pm/workspaces.mdx b/docs/pm/workspaces.mdx index 7b52fbb6cda1..368898a3779a 100644 --- a/docs/pm/workspaces.mdx +++ b/docs/pm/workspaces.mdx @@ -66,6 +66,8 @@ Each workspace has its own `package.json`. To reference another package in the m } ``` +A workspace needs a `"name"`. Bun skips a matched `package.json` that has no `"name"`, such as a test fixture containing only `{ "type": "module" }`. Bun does not link the skipped directory into `node_modules` and does not install its dependencies. If the skipped `package.json` declares dependencies, `bun install` prints a warning that names the directory. + `bun install` installs dependencies for all workspaces in the monorepo, de-duplicating packages if possible. To install dependencies for specific workspaces only, use the `--filter` flag. ```bash diff --git a/src/install/error.rs b/src/install/error.rs index 87ce091d1bca..57348c7db0ca 100644 --- a/src/install/error.rs +++ b/src/install/error.rs @@ -176,8 +176,6 @@ pub enum Error { CorruptLockfile, #[error("Lockfile is missing resolution data")] LockfileIsMissingResolutionData, - #[error("MissingPackageName")] - MissingPackageName, #[error("GlobError")] GlobError, #[error("Invalid")] @@ -343,7 +341,6 @@ impl Error { } Self::CorruptLockfile => "CorruptLockfile", Self::LockfileIsMissingResolutionData => "Lockfile is missing resolution data", - Self::MissingPackageName => "MissingPackageName", Self::GlobError => "GlobError", Self::Invalid => "Invalid", Self::LockfileValidationFailedListIsImpossiblyLong => { diff --git a/src/install/lockfile/Package.rs b/src/install/lockfile/Package.rs index fcd5b545bd4d..ff2eb22e60d7 100644 --- a/src/install/lockfile/Package.rs +++ b/src/install/lockfile/Package.rs @@ -2465,6 +2465,8 @@ impl Package { } } + workspace_names.warn_skipped(log); + if FEATURES.trusted_dependencies { if let Some(q) = json.as_property(b"trustedDependencies") { let count = match &q.expr.data { diff --git a/src/install/lockfile/Package/WorkspaceMap.rs b/src/install/lockfile/Package/WorkspaceMap.rs index 2f2d415ccd4d..0e4b11077cb2 100644 --- a/src/install/lockfile/Package/WorkspaceMap.rs +++ b/src/install/lockfile/Package/WorkspaceMap.rs @@ -8,6 +8,7 @@ use bun_paths as path; use bun_paths::resolve_path; use bun_paths::{MAX_PATH_BYTES, PathBuffer, SEP_STR}; +use super::DependencyGroup; use crate::lockfile_real::{Lockfile, StringBuilder, pruned_workspaces}; use crate::package_manager::workspace_package_json_cache::{ GetJSONOptions, WorkspacePackageJSONCache, @@ -17,6 +18,8 @@ bun_output::declare_scope!(Lockfile, hidden); pub(crate) struct WorkspaceMap { map: Map, + /// Relative dirs of nameless members that declare dependencies, for `warn_skipped`. + skipped_with_dependencies: Vec>, } type Map = StringArrayHashMap; @@ -28,10 +31,52 @@ pub struct Entry { pub(crate) name_loc: bun_ast::Loc, } +enum Scanned { + Workspace(Entry), + /// No usable `"name"`, so nothing to link or resolve it by; skipped, as pnpm and npm do. + Nameless { + declares_dependencies: bool, + }, +} + impl WorkspaceMap { pub(crate) fn init() -> WorkspaceMap { WorkspaceMap { map: Map::default(), + skipped_with_dependencies: Vec::new(), + } + } + + fn workspace_entry(&mut self, scanned: Scanned, relative_dir: &[u8]) -> Option { + match scanned { + Scanned::Workspace(entry) => Some(entry), + Scanned::Nameless { + declares_dependencies, + } => { + // Overlapping patterns match the same directory more than once. + if declares_dependencies + && !self + .skipped_with_dependencies + .iter() + .any(|dir| **dir == *relative_dir) + { + self.skipped_with_dependencies.push(relative_dir.into()); + } + None + } + } + } + + pub(crate) fn warn_skipped(&self, log: &mut bun_ast::Log) { + for dir in &self.skipped_with_dependencies { + log.add_warning_fmt( + None, + bun_ast::Loc::EMPTY, + format_args!( + "Skipping workspace \"{}\": its package.json has no \"name\", so its dependencies will not be installed", + BStr::new(dir) + ), + ); } } @@ -127,7 +172,7 @@ fn process_workspace_name( json_cache: &mut WorkspacePackageJSONCache, abs_package_json_path: &[u8], log: &mut bun_ast::Log, -) -> crate::Result { +) -> crate::Result { let workspace_json = json_cache .get_with_path( log, @@ -144,17 +189,32 @@ fn process_workspace_name( // results are immediately boxed so the bump can drop at scope exit. let scratch = Arena::new(); - let name_expr = workspace_json - .root - .get(b"name") - .ok_or(crate::Error::MissingPackageName)?; - let name = name_expr - .as_string_cloned(&scratch)? - .ok_or(crate::Error::MissingPackageName)?; + let name = match workspace_json.root.get(b"name") { + Some(name_expr) => name_expr + .as_string_cloned(&scratch)? + .filter(|name| !name.is_empty()) + .map(|name| (name, name_expr.loc)), + None => None, + }; + let Some((name, name_loc)) = name else { + bun_output::scoped_log!( + Lockfile, + "processWorkspaceName({}) has no name, skipping", + BStr::new(abs_package_json_path) + ); + return Ok(Scanned::Nameless { + declares_dependencies: DependencyGroup::FOUR.iter().any(|group| { + workspace_json + .root + .get(group.prop) + .is_some_and(|deps| deps.property_count() > 0) + }), + }); + }; let entry = Entry { name: Box::<[u8]>::from(name), - name_loc: name_expr.loc, + name_loc, version: 'brk: { if let Some(version_expr) = workspace_json.root.get(b"version") { if let Some(version) = version_expr.as_string_cloned(&scratch)? { @@ -171,7 +231,7 @@ fn process_workspace_name( BStr::new(&entry.name) ); - Ok(entry) + Ok(Scanned::Workspace(entry)) } fn workspace_dir_of(abs_package_json_path: &[u8]) -> &[u8] { @@ -265,12 +325,12 @@ impl WorkspaceMap { } process_workspace_name(json_cache, abs_package_json_path, log) - .map(|entry| (abs_package_json_path, entry)) + .map(|scanned| (abs_package_json_path, scanned)) } None => Err(crate::Error::Sys(bun_errno::SystemErrno::ENAMETOOLONG)), }; - let (abs_package_json_path, workspace_entry) = match processed { + let (abs_package_json_path, scanned) = match processed { Ok(processed) => processed, Err(err) => { if err == crate::Error::Sys(bun_errno::SystemErrno::ENOENT) { @@ -302,15 +362,6 @@ impl WorkspaceMap { arr.item_loc(source, i), format_args!("Workspace not found \"{}\"", BStr::new(input_path)), ); - } else if err == crate::Error::MissingPackageName { - let _ = log.add_error_fmt( - Some(source), - loc, - format_args!( - "Missing \"name\" from package.json in {}", - BStr::new(input_path) - ), - ); } else { let mut cwd_buf = vec![0u8; MAX_PATH_BYTES]; let cwd_len = bun_sys::getcwd(&mut cwd_buf).expect("unreachable"); @@ -329,16 +380,17 @@ impl WorkspaceMap { } }; - if workspace_entry.name.len() == 0 { - continue; - } - let rel_input_path = relative_workspace_path( &mut rel_path_buf.0, root_dir, workspace_dir_of(abs_package_json_path), ); + let Some(workspace_entry) = workspace_names.workspace_entry(scanned, rel_input_path) + else { + continue; + }; + if let Some(builder) = string_builder.as_deref_mut() { builder.count(&workspace_entry.name); builder.count(rel_input_path); @@ -484,55 +536,44 @@ impl WorkspaceMap { ) { Some(abs_package_json_path) => { process_workspace_name(json_cache, abs_package_json_path, log) - .map(|entry| (abs_package_json_path, entry)) + .map(|scanned| (abs_package_json_path, scanned)) } None => Err(crate::Error::Sys(bun_errno::SystemErrno::ENAMETOOLONG)), }; - let (abs_package_json_path, workspace_entry) = match processed { + let (abs_package_json_path, scanned) = match processed { Ok(processed) => processed, Err(err) => { - let entry_base: &[u8] = path::basename(matched_path); if err == crate::Error::Sys(bun_errno::SystemErrno::ENOENT) { continue; - } else if err == crate::Error::MissingPackageName { - let _ = log.add_error_fmt( - Some(source), - bun_ast::Loc::EMPTY, - format_args!( - "Missing \"name\" from package.json in {}{}{}", - BStr::new(entry_dir), - SEP_STR, - BStr::new(entry_base), - ), - ); - } else { - let _ = log.add_error_fmt( - Some(source), - bun_ast::Loc::EMPTY, - format_args!( - "{} reading package.json for workspace package \"{}\" from \"{}\"", - err.name(), - BStr::new(entry_dir), - BStr::new(entry_base), - ), - ); } - + let entry_base: &[u8] = path::basename(matched_path); + let _ = log.add_error_fmt( + Some(source), + bun_ast::Loc::EMPTY, + format_args!( + "{} reading package.json for workspace package \"{}\" from \"{}\"", + err.name(), + BStr::new(entry_dir), + BStr::new(entry_base), + ), + ); continue; } }; - if workspace_entry.name.len() == 0 { - continue; - } - let workspace_path: &[u8] = relative_workspace_path( &mut rel_path_buf.0, root_dir, workspace_dir_of(abs_package_json_path), ); + let Some(workspace_entry) = + workspace_names.workspace_entry(scanned, workspace_path) + else { + continue; + }; + if let Some(builder) = string_builder.as_deref_mut() { builder.count(&workspace_entry.name); builder.count(workspace_path); diff --git a/src/install/migration.rs b/src/install/migration.rs index dd5270fe8424..3257e9c0cbb1 100644 --- a/src/install/migration.rs +++ b/src/install/migration.rs @@ -133,11 +133,6 @@ pub fn detect_and_load_other_lockfile<'a>( "Relative link dependencies aren't supported yet. Please follow along at https://github.com/oven-sh/bun/issues/23026", ); } - MigratePnpmLockfileError::WorkspaceNameMissing => { - bun_core::warn!( - "pnpm-lock.yaml migration failed due to missing workspace name.", - ); - } MigratePnpmLockfileError::YamlParseError => { bun_core::warn!("Failed to parse pnpm-lock.yaml."); } diff --git a/src/install/pnpm.rs b/src/install/pnpm.rs index abf7916c98f8..7eb974ee69a5 100644 --- a/src/install/pnpm.rs +++ b/src/install/pnpm.rs @@ -364,8 +364,6 @@ pub enum MigratePnpmLockfileError { NonExistentWorkspaceDependency, #[error("RelativeLinkDependency")] RelativeLinkDependency, - #[error("WorkspaceNameMissing")] - WorkspaceNameMissing, #[error("DependencyLoop")] DependencyLoop, #[error("PnpmLockfileNotObject")] @@ -770,9 +768,11 @@ pub(crate) fn migrate_pnpm_lockfile<'a>( let workspace_root = &importer_pkg_json.root; - let Some((name, _)) = get_string(workspace_root, b"name") else { - // we require workspace names. - return Err(MigratePnpmLockfileError::WorkspaceNameMissing); + // Nameless importers are skipped like `WorkspaceMap::process_workspace_name` skips them. + let Some((name, _)) = + get_string(workspace_root, b"name").filter(|(name, _)| !name.is_empty()) + else { + continue; }; let name_hash = semver::string::Builder::string_hash(name); diff --git a/test/cli/install/bun-workspaces.test.ts b/test/cli/install/bun-workspaces.test.ts index 48903d67ec9f..15fabd48885f 100644 --- a/test/cli/install/bun-workspaces.test.ts +++ b/test/cli/install/bun-workspaces.test.ts @@ -233,6 +233,96 @@ test.concurrent("allowing negative workspace patterns", async () => { }); }); +// pnpm and npm accept workspace members whose package.json has no "name" (vite's +// pnpm-workspace.yaml matches dozens of test fixtures shaped like `{"type":"module"}`). +describe("workspace member package.json without a name", () => { + const skippedWarning = + 'warn: Skipping workspace "packages/fixture": its package.json has no "name", so its dependencies will not be installed\n'; + + async function setupMonorepo(packageDir: string, workspaces: string[], fixturePackageJson: object) { + await Promise.all([ + write( + join(packageDir, "package.json"), + JSON.stringify({ + name: "root", + workspaces, + dependencies: { + pkg1: "workspace:*", + }, + }), + ), + write(join(packageDir, "packages", "pkg1", "package.json"), JSON.stringify({ name: "pkg1", version: "1.0.0" })), + write(join(packageDir, "packages", "fixture", "package.json"), JSON.stringify(fixturePackageJson)), + ]); + } + + async function expectFixtureSkipped(packageDir: string) { + expect(await file(join(packageDir, "node_modules", "pkg1", "package.json")).json()).toEqual({ + name: "pkg1", + version: "1.0.0", + }); + expect(await exists(join(packageDir, "node_modules", "fixture"))).toBeFalse(); + const lockfile = await file(join(packageDir, "bun.lock")).text(); + expect(lockfile).toContain('"packages/pkg1"'); + expect(lockfile).not.toContain("packages/fixture"); + } + + const patterns = { + glob: ["packages/*"], + listed: ["packages/pkg1", "packages/fixture"], + // The fixture matches three times; it must still be warned about once. + overlapping: ["packages/fixture", "packages/*", "packages/**"], + }; + + for (const [kind, workspaces] of Object.entries(patterns)) { + test.concurrent(`is skipped (${kind})`, async () => { + using ctx = await setupTest(); + const { packageDir, env } = ctx; + await setupMonorepo(packageDir, workspaces, { type: "module" }); + + // runBunInstall asserts nothing was warned about. + await runBunInstall(env, packageDir); + await expectFixtureSkipped(packageDir); + }); + + test.concurrent(`is skipped with a warning when it declares dependencies (${kind})`, async () => { + using ctx = await setupTest(); + const { packageDir, env } = ctx; + // Resolving this dependency would fail the install, so a successful install + // proves the fixture's dependencies were never looked at. + await setupMonorepo(packageDir, workspaces, { devDependencies: { "doesnt-exist-oops": "1.2.3" } }); + + const { err } = await runBunInstall(env, packageDir, { allowWarnings: true }); + expect(err).toContain(skippedWarning); + expect(err.split(skippedWarning)).toHaveLength(2); + await expectFixtureSkipped(packageDir); + }); + } + + test.concurrent("an empty name counts as no name", async () => { + using ctx = await setupTest(); + const { packageDir, env } = ctx; + await setupMonorepo(packageDir, patterns.glob, { name: "", dependencies: { "doesnt-exist-oops": "1.2.3" } }); + + const { err } = await runBunInstall(env, packageDir, { allowWarnings: true }); + expect(err).toContain(skippedWarning); + await expectFixtureSkipped(packageDir); + }); + + test.concurrent("does not stop a member from finding the workspace root", async () => { + using ctx = await setupTest(); + const { packageDir, env } = ctx; + await setupMonorepo(packageDir, patterns.glob, { dependencies: { "doesnt-exist-oops": "1.2.3" } }); + + const { err } = await runBunInstall(env, join(packageDir, "packages", "pkg1"), { allowWarnings: true }); + + expect(await exists(join(packageDir, "packages", "pkg1", "bun.lock"))).toBeFalse(); + // Both the root lookup and the root package.json parse scan the workspaces; only the latter warns. + expect(err.split(skippedWarning)).toHaveLength(2); + await expectFixtureSkipped(packageDir); + }); +}); + test("dependency on same name as workspace and dist-tag", async () => { using ctx = await setupTest(); const { packageDir, env } = ctx; diff --git a/test/cli/install/migration/__snapshots__/migrate.test.ts.snap b/test/cli/install/migration/__snapshots__/migrate.test.ts.snap index 6b83d448d907..24abf4c88e05 100644 --- a/test/cli/install/migration/__snapshots__/migrate.test.ts.snap +++ b/test/cli/install/migration/__snapshots__/migrate.test.ts.snap @@ -4660,8 +4660,26 @@ bun install --frozen-lockfile exit code: 0 `; exports[`package-lock.json migration fixes arborist fixtures workspaces-need-update 1`] = ` -"error: failed to migrate lockfile: InstallFailed -bun pm migrate exit code: 1" +"warn: skipped 4 package-lock.json entries not depended on by any package: "a", "b", "node_modules/once", "node_modules/wrappy" +migrated lockfile from package-lock.json +bun pm migrate exit code: 0 +bun install --frozen-lockfile exit code: 0 + +{ + "lockfileVersion": 2, + "configVersion": 1, + "workspaces": { + "": { + "dependencies": { + "abbrev": "^1.0.4", + }, + }, + }, + "packages": { + "abbrev": ["abbrev@1.0.4", "", {}, "sha1-vVWuXkE7oXIu5Mq6H26hBBSlns0="], + } +} +" `; exports[`package-lock.json migration fixes arborist fixtures workspaces-non-simplistic 1`] = ` diff --git a/test/cli/install/migration/npm-arborist/README.md b/test/cli/install/migration/npm-arborist/README.md index 8e3f17dc5c94..a05d240dcc0d 100644 --- a/test/cli/install/migration/npm-arborist/README.md +++ b/test/cli/install/migration/npm-arborist/README.md @@ -53,7 +53,7 @@ Each directory holds only what a `package-lock.json -> bun.lock` migration reads - `workspaces-add-new-dep` (v2, workspaces) - single workspace `a` at the fixture root, no registry deps - `workspaces-conflicting-versions-virtual` (v2, workspaces) - two workspaces pinning different versions of the root's dep, nested under each workspace - `workspaces-ignore-nm-virtual` (v2, workspaces) - workspaces glob `packages/**` -- `workspaces-need-update` (v2, workspaces) - workspaces a/b whose package.json files have no `name` (Bun rejects nameless workspaces) +- `workspaces-need-update` (v2, workspaces) - workspaces a/b whose package.json files have no `name` (Bun skips nameless workspaces, so only the root's deps migrate) - `workspaces-non-simplistic` (v2, workspaces) - workspace with a scoped transitive dep chain, minified lockfile, root devDependencies - `workspaces-not-root` (v2, workspaces) - three workspaces sharing hoisted registry deps, `""` specs, root package.json without a name - `workspaces-prefer-linking-virtual` (v2, workspaces) - workspace named `abbrev` satisfying another workspace's `abbrev` dep diff --git a/test/cli/install/migration/npm-arborist/fixtures.json b/test/cli/install/migration/npm-arborist/fixtures.json index 7f9dd08de44c..47f1a43ef1de 100644 --- a/test/cli/install/migration/npm-arborist/fixtures.json +++ b/test/cli/install/migration/npm-arborist/fixtures.json @@ -335,7 +335,7 @@ "lockfile": "package-lock.json", "lockfileVersion": 2, "workspaces": true, - "notes": "workspaces a/b whose package.json files have no `name` (Bun rejects nameless workspaces)" + "notes": "workspaces a/b whose package.json files have no `name` (Bun skips nameless workspaces, so only the root's deps migrate)" }, { "name": "workspaces-non-simplistic", diff --git a/test/cli/install/migration/pnpm-lock-v9.test.ts b/test/cli/install/migration/pnpm-lock-v9.test.ts index 9fe4c0434e27..6e3b2703fdd5 100644 --- a/test/cli/install/migration/pnpm-lock-v9.test.ts +++ b/test/cli/install/migration/pnpm-lock-v9.test.ts @@ -234,6 +234,111 @@ describe("pnpm-lock.yaml v9", () => { expect(existsSync(join(String(dir), "bun.lock"))).toBe(false); }); + // pnpm lists every directory matched by pnpm-workspace.yaml as an importer, including + // test fixtures whose package.json has no "name" (vite has dozens). Those are not + // workspace packages to bun; the migration and the later workspace scan both skip them. + describe("importer whose package.json has no name", () => { + const fixtureDir = "packages/a/__tests__/fixtures/esm"; + const files = { + "package.json": JSON.stringify({ + name: "nameless-importer-root", + private: true, + dependencies: { a: "workspace:*" }, + }), + "pnpm-workspace.yaml": "packages:\n - 'packages/*'\n - 'packages/**/__tests__/**'\n", + "packages/a/package.json": JSON.stringify({ name: "a" }), + }; + const rootImporter = `lockfileVersion: '9.0' + +importers: + + .: + dependencies: + a: + specifier: workspace:* + version: link:packages/a + + packages/a: {} +`; + + async function migrateThenFrozenInstall(dir: string) { + const { stderr, exitCode } = await migrate(dir); + expect(stderr).toContain("moved pnpm-workspace.yaml to workspaces in package.json"); + expect(stderr).toContain("migrated lockfile from pnpm-lock.yaml"); + expect(stderr).not.toContain("warn:"); + expect(exitCode).toBe(0); + + const migrated = await bunLockOf(dir); + expect(workspacesSection(migrated)).toBe(` "workspaces": { + "": { + "name": "nameless-importer-root", + "dependencies": { + "a": "workspace:*", + }, + }, + "packages/a": { + "name": "a", + }, + }, +`); + expect((await Bun.file(join(dir, "package.json")).json()).workspaces).toStrictEqual([ + "packages/*", + "packages/**/__tests__/**", + ]); + + // The migrated package.json now matches the fixture through the glob; the + // workspace scan has to agree with the migration for a frozen install to pass. + const install = await run(dir, "install", "--frozen-lockfile", "--linker", "hoisted"); + expect(install.stderr).not.toContain("error:"); + expect(install.exitCode).toBe(0); + expect(await bunLockOf(dir)).toBe(migrated); + expect(await installedPackageJson(dir, "", "a")).toStrictEqual({ name: "a" }); + return { migrated, install }; + } + + test.concurrent("is skipped", async () => { + using dir = tempDir("pnpm-v9-nameless-importer", { + ...files, + [`${fixtureDir}/package.json`]: JSON.stringify({ type: "module" }), + "pnpm-lock.yaml": `${rootImporter} + ${fixtureDir}: {} +`, + }); + + const { install } = await migrateThenFrozenInstall(String(dir)); + expect(install.stderr).not.toContain("warn:"); + }); + + test.concurrent("its dependencies are not migrated, and bun install warns about them", async () => { + using dir = tempDir("pnpm-v9-nameless-importer-with-deps", { + ...files, + [`${fixtureDir}/package.json`]: JSON.stringify({ type: "module", dependencies: { "no-deps": "^1.0.0" } }), + "pnpm-lock.yaml": `${rootImporter} + ${fixtureDir}: + dependencies: + no-deps: + specifier: ^1.0.0 + version: 1.0.1 + +packages: + + no-deps@1.0.1: + resolution: {integrity: ${NO_DEPS_1_0_1_INTEGRITY}} + +snapshots: + + no-deps@1.0.1: {} +`, + }); + + const { migrated, install } = await migrateThenFrozenInstall(String(dir)); + expect(migrated).not.toContain("no-deps"); + expect(install.stderr).toContain( + `warn: Skipping workspace "${fixtureDir}": its package.json has no "name", so its dependencies will not be installed\n`, + ); + }); + }); + test("registry-qualified dep path resolves from the configured registry with a warning", async () => { // shape from pnpm11/deps/path/test/index.ts parse() `foo@work:1.0.0` const { packageDir } = await verdaccio.createTestDir({