Assorted small fixes and cleanup (#865)

* Add a constant for the change folder

* Add change/* to ignorePatterns automatically

* Typing and cleanup for publish overrides

* Improve publish bump/push logging and correctly detect git timeouts

* Fix line breaks in writeChangeFiles logging

* minor getChangedPackages cleanup

* add process.exit debugging in tests

beachball.config.js

@@ -10,6 +10,7 @@ module.exports = {
     'docs/**',
     'jest.*.js',
     'renovate.json5',
+    'scripts/**',
     'src/__*/**',
     // This one is especially important (otherwise dependabot would be blocked by change file requirements)
     'yarn.lock',

change/beachball-2bae994a-b015-4a5c-b1c9-2e57fc384d59.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Improve publish bump/push logging and correctly detect git timeouts",
+  "packageName": "beachball",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

change/beachball-a6924256-243d-423f-a19f-75860a9aeb0c.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Fix line breaks in writeChangeFiles logging",
+  "packageName": "beachball",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

change/beachball-d7523f70-dbe3-478e-9c34-055f1142440d.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "When determining changed packages, exclude change files as part of ignorePatterns",
+  "packageName": "beachball",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

change/beachball-f53b5509-d497-4a8c-bf22-de4a87c2e446.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Minor cleanup for publish overrides, getChangedPackages, etc",
+  "packageName": "beachball",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

jest.config.js

@@ -1,15 +1,17 @@
 // @ts-check
 /** @type {import('@jest/types').Config.InitialProjectOptions} */
 const commonOptions = {
+  injectGlobals: false,
   roots: ['<rootDir>/src'],
+  setupFilesAfterEnv: ['<rootDir>/scripts/jestSetup.js'],
   transform: {
     '^.+\\.tsx?$': 'ts-jest',
   },
   testEnvironment: 'node',
 };
 
 /** @type {import('@jest/types').Config.InitialOptions} */
-module.exports = {
+const config = {
   reporters: ['default', 'github-actions'],
   testTimeout: 60000,
   projects: [
@@ -30,3 +32,4 @@ module.exports = {
     },
   ],
 };
+module.exports = config;

package.json

@@ -51,7 +51,7 @@
     "semver": "^7.0.0",
     "toposort": "^2.0.2",
     "uuid": "^9.0.0",
-    "workspace-tools": "^0.34.0",
+    "workspace-tools": "^0.34.2",
     "yargs-parser": "^21.0.0"
   },
   "devDependencies": {

scripts/jestSetup.js

@@ -0,0 +1,6 @@
+// @ts-check
+const { jest } = require('@jest/globals');
+
+jest.spyOn(process, 'exit').mockImplementation(code => {
+  throw new Error(`process.exit called with code ${code}`);
+});

src/changefile/getChangedPackages.ts

@@ -2,7 +2,7 @@ import fs from 'fs-extra';
 import path from 'path';
 import minimatch from 'minimatch';
 import { ChangeFileInfo, ChangeInfoMultiple } from '../types/ChangeInfo';
-import { getChangePath } from '../paths';
+import { changeFolder, getChangePath } from '../paths';
 import { getChanges, getStagedChanges, git, fetchRemoteBranch, parseRemoteBranch } from 'workspace-tools';
 import { getScopedPackages } from '../monorepo/getScopedPackages';
 import { BeachballOptions } from '../types/BeachballOptions';
@@ -125,8 +125,8 @@ function getAllChangedPackages(options: BeachballOptions, packageInfos: PackageI
   }
 
   // Filter out changed files which are ignored by ignorePatterns.
-  // Also ignore the CHANGELOG files because they're generated by beachball.
-  const ignorePatterns = [...(options.ignorePatterns || []), 'CHANGELOG.md', 'CHANGELOG.json'];
+  // Also ignore the CHANGELOG files and change files because they're generated by beachball.
+  const ignorePatterns = [...(options.ignorePatterns || []), `${changeFolder}/*.json`, 'CHANGELOG.{md,json}'];
   const nonIgnoredChanges = changes.filter(moddedFile => {
     const ignorePattern = ignorePatterns.find(pattern => minimatch(moddedFile, pattern, { matchBase: true }));
     ignorePattern && logIgnored(moddedFile, `ignored by pattern "${ignorePattern}"`);
@@ -195,25 +195,22 @@ export function getChangedPackages(options: BeachballOptions, packageInfos: Pack
   }
 
   const changes = changeFilesResult.stdout.split(/\n/);
-  const changeFiles = changes.filter(name => path.dirname(name) === 'change');
+  const changeFiles = changes.filter(name => path.dirname(name) === changeFolder);
   const changeFilePackageSet = new Set<string>();
 
   // Loop through the change files, building up a set of packages that we can skip
-  changeFiles.forEach(file => {
+  for (const file of changeFiles) {
     try {
-      const changeInfo: ChangeFileInfo | ChangeInfoMultiple = fs.readJSONSync(file);
+      const changeInfo: ChangeFileInfo | ChangeInfoMultiple = fs.readJSONSync(path.join(cwd, file));
+      const changes = (changeInfo as ChangeInfoMultiple).changes || [changeInfo];
 
-      if ('changes' in changeInfo) {
-        for (const change of (changeInfo as ChangeInfoMultiple).changes) {
-          changeFilePackageSet.add(change.packageName);
-        }
-      } else {
-        changeFilePackageSet.add((changeInfo as ChangeFileInfo).packageName);
+      for (const change of changes) {
+        changeFilePackageSet.add(change.packageName);
       }
     } catch (e) {
       console.warn(`Error reading or parsing change file ${file}: ${e}`);
     }
-  });
+  }
 
   if (changeFilePackageSet.size > 0) {
     console.log(

src/changefile/writeChangeFiles.ts

@@ -58,8 +58,8 @@ export function writeChangeFiles({
 
   console.log(
     `git ${commitChangeFiles ? 'committed' : 'staged'} these change files: ${changeFiles
-      .map(f => ` - ${f}`)
-      .join('\n')}`
+      .map(f => `\n - ${f}`)
+      .join('')}`
   );
 
   return changeFiles;

src/paths.ts

@@ -1,10 +1,13 @@
 import path from 'path';
 import { findProjectRoot } from 'workspace-tools';
 
+/** Relative path to the change files folder */
+export const changeFolder = 'change';
+
 /**
- * Get the folder containing beachball change files.
+ * Get the absolute path to the folder containing beachball change files.
  */
 export function getChangePath(cwd: string) {
   const root = findProjectRoot(cwd);
-  return path.join(root, 'change');
+  return path.join(root, changeFolder);
 }

src/publish/bumpAndPush.ts

@@ -9,7 +9,7 @@ import { displayManualRecovery } from './displayManualRecovery';
 const BUMP_PUSH_RETRIES = 5;
 
 export async function bumpAndPush(bumpInfo: BumpInfo, publishBranch: string, options: BeachballOptions) {
-  const { path: cwd, branch, tag, message, gitTimeout } = options;
+  const { path: cwd, branch, tag, depth, message, gitTimeout } = options;
   const { remote, remoteBranch } = parseRemoteBranch(branch);
 
   let completed = false;
@@ -20,27 +20,21 @@ export async function bumpAndPush(bumpInfo: BumpInfo, publishBranch: string, opt
     console.log(`Trying to push to git. Attempt ${tryNumber}/${BUMP_PUSH_RETRIES}`);
     console.log('Reverting');
     revertLocalChanges(cwd);
+    const warnPrefix = `[WARN ${tryNumber}/${BUMP_PUSH_RETRIES}]:`;
 
     // pull in latest from origin branch
     if (options.fetch !== false) {
       console.log('Fetching from remote');
-      let fetchResult;
-      if (options.depth) {
-        fetchResult = git(['fetch', remote, remoteBranch, `--depth=${options.depth}`], { cwd });
-      } else {
-        fetchResult = git(['fetch', remote, remoteBranch], { cwd });
-      }
+      const fetchResult = git(['fetch', remote, remoteBranch, ...(depth ? [`--depth=${depth}`] : [])], { cwd });
       if (!fetchResult.success) {
-        console.warn(
-          `[WARN ${tryNumber}/${BUMP_PUSH_RETRIES}]: fetch from ${branch} has failed!\n${fetchResult.stderr}`
-        );
+        console.warn(`${warnPrefix} fetch from ${branch} has failed!\n${fetchResult.stderr}`);
         continue;
       }
     }
 
     const mergeResult = git(['merge', '-X', 'theirs', `${branch}`], { cwd });
     if (!mergeResult.success) {
-      console.warn(`[WARN ${tryNumber}/${BUMP_PUSH_RETRIES}]: pull from ${branch} has failed!\n${mergeResult.stderr}`);
+      console.warn(`${warnPrefix} pull from ${branch} has failed!\n${mergeResult.stderr}`);
       continue;
     }
 
@@ -51,7 +45,7 @@ export async function bumpAndPush(bumpInfo: BumpInfo, publishBranch: string, opt
     // checkin
     const mergePublishBranchResult = await mergePublishBranch(publishBranch, branch, message, cwd, options);
     if (!mergePublishBranchResult.success) {
-      console.warn(`[WARN ${tryNumber}/${BUMP_PUSH_RETRIES}]: merging to target has failed!`);
+      console.warn(`${warnPrefix} merging to target has failed!`);
       continue;
     }
 
@@ -69,16 +63,21 @@ export async function bumpAndPush(bumpInfo: BumpInfo, publishBranch: string, opt
       const pushResult = git(pushArgs, { cwd, timeout: gitTimeout });
 
       if (!pushResult.success) {
-        console.warn(`[WARN ${tryNumber}/${BUMP_PUSH_RETRIES}]: push to ${branch} has failed!\n${pushResult.stderr}`);
-        continue;
+        // If it timed out, the return value contains an "error" object with ETIMEDOUT code
+        // (it doesn't throw the error)
+        if ((pushResult.error as any)?.code === 'ETIMEDOUT') {
+          console.warn(`${warnPrefix} push to ${branch} has timed out!`);
+        } else {
+          console.warn(`${warnPrefix} push to ${branch} has failed!\n${pushResult.stderr}`);
+        }
       } else {
         console.log(pushResult.stdout.toString());
         console.log(pushResult.stderr.toString());
         completed = true;
       }
     } catch (e) {
-      console.warn(`[WARN ${tryNumber}/${BUMP_PUSH_RETRIES}]: push to ${branch} has failed!\n${e}`);
-      continue;
+      // This is likely not reachable (see comment above), but leaving it just in case for this critical operation
+      console.warn(`${warnPrefix} push to ${branch} has failed!\n${e}`);
     }
   }
 

src/publish/performPublishOverrides.ts

@@ -1,7 +1,18 @@
-import { PackageInfos } from '../types/PackageInfo';
+import { PackageInfos, PackageJson } from '../types/PackageInfo';
 import * as fs from 'fs-extra';
 
-export const acceptedKeys = ['types', 'typings', 'main', 'module', 'exports', 'repository', 'bin', 'browser', 'files'];
+const acceptedKeys = [
+  'types',
+  'typings',
+  'main',
+  'module',
+  'exports',
+  'repository',
+  'bin',
+  'browser',
+  'files',
+] as const;
+const workspacePrefix = 'workspace:';
 
 export function performPublishOverrides(packagesToPublish: string[], packageInfos: PackageInfos) {
   for (const pkgName of packagesToPublish) {
@@ -15,13 +26,15 @@ export function performPublishOverrides(packagesToPublish: string[], packageInfo
   }
 }
 
-function performPublishConfigOverrides(packageJson: any) {
+function performPublishConfigOverrides(packageJson: PackageJson) {
   // Everything in publishConfig in accepted keys here will get overridden & removed from the publishConfig section
   if (packageJson.publishConfig) {
-    for (const key of acceptedKeys) {
-      const value = packageJson.publishConfig[key] || packageJson[key];
-      packageJson[key] = value;
-      delete packageJson.publishConfig[key];
+    for (const [k, value] of Object.entries(packageJson.publishConfig)) {
+      const key = k as keyof Required<PackageJson>['publishConfig'];
+      if (acceptedKeys.includes(key)) {
+        packageJson[key] = value;
+        delete packageJson.publishConfig[key];
+      }
     }
   }
 }
@@ -32,19 +45,18 @@ function performPublishConfigOverrides(packageJson: any) {
  * handle this replacement for us, but as of this time publishing only happens via npm, which can't do this
  * replacement.
  */
-function performWorkspaceVersionOverrides(packageJson: any, packageInfos: PackageInfos) {
-  const depTypes = ['dependencies', 'devDependencies', 'peerDependencies'] as const;
-  depTypes.forEach(depKind => {
-    const deps = packageJson[depKind];
-    if (deps) {
-      Object.keys(deps).forEach(dep => {
-        const packageInfo = packageInfos[dep];
-        if (packageInfo && deps[dep].startsWith('workspace:')) {
-          deps[dep] = resolveWorkspaceVersionForPublish(deps[dep], packageInfo.version);
-        }
-      });
+function performWorkspaceVersionOverrides(packageJson: PackageJson, packageInfos: PackageInfos) {
+  const { dependencies, devDependencies, peerDependencies } = packageJson;
+  for (const deps of [dependencies, devDependencies, peerDependencies]) {
+    if (!deps) continue;
+
+    for (const [depName, depVersion] of Object.entries(deps)) {
+      const packageInfo = packageInfos[depName];
+      if (packageInfo && depVersion.startsWith(workspacePrefix)) {
+        deps[depName] = resolveWorkspaceVersionForPublish(depVersion, packageInfo.version);
+      }
     }
-  });
+  }
 }
 
 /**
@@ -53,12 +65,12 @@ function performWorkspaceVersionOverrides(packageJson: any, packageInfos: Packag
  * https://yarnpkg.com/features/workspaces#publishing-workspaces
  */
 function resolveWorkspaceVersionForPublish(workspaceDependency: string, packageInfoVersion: string): string {
-  const versionStartIndex = 'workspace:'.length;
-  if (workspaceDependency === 'workspace:*') {
+  const workspaceVersion = workspaceDependency.substring(workspacePrefix.length);
+  if (workspaceVersion === '*') {
     return packageInfoVersion;
-  } else if (new Set(['workspace:~', 'workspace:^']).has(workspaceDependency)) {
-    return `${workspaceDependency[versionStartIndex]}${packageInfoVersion}`;
-  } else {
-    return workspaceDependency.substring(versionStartIndex);
   }
+  if (workspaceVersion === '^' || workspaceVersion === '~') {
+    return `${workspaceVersion}${packageInfoVersion}`;
+  }
+  return workspaceVersion;
 }

src/types/PackageInfo.ts

@@ -9,11 +9,24 @@ export interface PackageJson {
   name: string;
   version: string;
   main?: string;
+  module?: string;
+  types?: string;
+  typings?: string;
+  exports?: any;
+  repository?: any;
+  bin?: any;
+  browser?: any;
+  files?: string[];
   dependencies?: PackageDeps;
   devDependencies?: PackageDeps;
   peerDependencies?: PackageDeps;
   private?: boolean;
   beachball?: BeachballOptions;
+  /** Overrides applied during publishing */
+  publishConfig?: Pick<
+    PackageJson,
+    'types' | 'typings' | 'main' | 'module' | 'exports' | 'repository' | 'bin' | 'browser' | 'files'
+  >;
 }
 
 export interface PackageInfo {

yarn.lock

@@ -11598,10 +11598,10 @@ worker-farm@^1.7.0:
   dependencies:
     errno "~0.1.7"
 
-workspace-tools@^0.34.0:
-  version "0.34.0"
-  resolved "https://registry.yarnpkg.com/workspace-tools/-/workspace-tools-0.34.0.tgz#cfe096d8d55c8c3ec7c7977b84bbe2bbdfb05e17"
-  integrity sha512-grYUZvWs7SXJzXCPx0jdIkErFkGKVt74jZKYC2GxIEX2IZgEYYjwLmWDIPnZAtXpQ2jNuZiXxh9Kglyb5LUpnQ==
+workspace-tools@^0.34.2:
+  version "0.34.2"
+  resolved "https://registry.yarnpkg.com/workspace-tools/-/workspace-tools-0.34.2.tgz#c45f0334f1d12001d574ef54cc37f02cecd3cf4c"
+  integrity sha512-mrGVKGTjOhMPmkrBZ2oL7GytAkAJe9Nhat7GfkIoCNk6aQexBnKrjqDtnGyQneX7Lx5Rcv/TOPyd6aGC0fvXZg==
   dependencies:
     "@yarnpkg/lockfile" "^1.1.0"
     git-url-parse "^13.0.0"