Remove newPackages from main BumpInfo (#990)

change/beachball-848fed39-4933-483a-a038-1470bebe299b.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Remove newPackages from main BumpInfo",
+  "packageName": "beachball",
+  "email": "elcraig@microsoft.com",
+  "dependentChangeType": "patch"
+}

src/__tests__/bump/updateRelatedChangeType.test.ts

@@ -26,7 +26,6 @@ describe('updateRelatedChangeType', () => {
           baz: { combinedOptions: { disallowedChangeTypes: [], defaultNpmTag: 'latest' } },
         }),
         modifiedPackages: new Set(),
-        newPackages: new Set(),
         packageGroups: {},
         scopedPackages: new Set(),
         dependentChangedBy: {},

src/__tests__/publish/tagPackages.test.ts

@@ -4,7 +4,6 @@ import { gitFailFast } from 'workspace-tools';
 import { initMockLogs } from '../../__fixtures__/mockLogs';
 import { tagPackages } from '../../publish/tagPackages';
 import { generateTag } from '../../git/generateTag';
-import { BumpInfo } from '../../types/BumpInfo';
 import { makePackageInfos } from '../../__fixtures__/packageInfos';
 
 jest.mock('workspace-tools', () => ({
@@ -15,7 +14,10 @@ const createTagParameters = (tag: string, cwd: string) => {
   return [['tag', '-a', '-f', tag, '-m', tag], { cwd }];
 };
 
-const noTagBumpInfo: Partial<BumpInfo> = {
+type TagBumpInfo = Parameters<typeof tagPackages>[0];
+
+/** foo and bar disable gitTags */
+const noTagBumpInfo: TagBumpInfo = {
   calculatedChangeTypes: {
     foo: 'minor',
     bar: 'major',
@@ -31,14 +33,12 @@ const noTagBumpInfo: Partial<BumpInfo> = {
     },
   }),
   modifiedPackages: new Set(['foo', 'bar']),
-  newPackages: new Set(),
+  newPackages: [],
 };
 
-const oneTagBumpInfo: Partial<BumpInfo> = {
-  calculatedChangeTypes: {
-    foo: 'minor',
-    bar: 'major',
-  },
+/** foo enables gitTags, bar disables it */
+const oneTagBumpInfo: TagBumpInfo = {
+  ...noTagBumpInfo,
   packageInfos: makePackageInfos({
     foo: {
       version: '1.0.0',
@@ -49,15 +49,13 @@ const oneTagBumpInfo: Partial<BumpInfo> = {
       combinedOptions: { gitTags: false },
     },
   }),
-  modifiedPackages: new Set(['foo', 'bar']),
-  newPackages: new Set(),
 };
 
-const emptyBumpInfo: Partial<BumpInfo> = {
+const emptyBumpInfo: TagBumpInfo = {
   calculatedChangeTypes: {},
   packageInfos: {},
   modifiedPackages: new Set(),
-  newPackages: new Set(),
+  newPackages: [],
 };
 
 describe('tagPackages', () => {
@@ -71,14 +69,14 @@ describe('tagPackages', () => {
     jest.restoreAllMocks();
   });
 
-  it('does not create tag for packages with gitTags=false', () => {
+  it('does not create package tag for packages with gitTags=false', () => {
     // Also verifies that if `gitTags` is false overall, it doesn't create a git tag for the dist tag (`tag`)
-    tagPackages(noTagBumpInfo as BumpInfo, { path: '', gitTags: false, tag: '' });
+    tagPackages(noTagBumpInfo, { path: '', gitTags: false, tag: '' });
     expect(gitFailFast).not.toHaveBeenCalled();
   });
 
-  it('creates tag for packages with gitTags=true', () => {
-    tagPackages(oneTagBumpInfo as BumpInfo, { path: '', gitTags: false, tag: '' });
+  it('creates package tag for packages with gitTags=true', () => {
+    tagPackages(oneTagBumpInfo, { path: '', gitTags: false, tag: '' });
     expect(gitFailFast).toHaveBeenCalledTimes(1);
 
     // verify git is being called to create new auto tag for foo and bar
@@ -86,18 +84,35 @@ describe('tagPackages', () => {
     expect(gitFailFast).toHaveBeenCalledWith(...createTagParameters(newFooTag, ''));
   });
 
-  it('does not create git tag for empty dist tag', () => {
-    tagPackages(emptyBumpInfo as BumpInfo, { path: '', gitTags: true, tag: '' });
+  it('creates package tag for new packages with gitTags=true', () => {
+    tagPackages(
+      { ...oneTagBumpInfo, newPackages: ['foo'], modifiedPackages: new Set() },
+      { path: '', gitTags: false, tag: '' }
+    );
+    expect(gitFailFast).toHaveBeenCalledTimes(1);
+
+    // verify git is being called to create new auto tag for foo
+    const newFooTag = generateTag('foo', oneTagBumpInfo.packageInfos!['foo'].version);
+    expect(gitFailFast).toHaveBeenCalledWith(...createTagParameters(newFooTag, ''));
+  });
+
+  it('does not create package tag for packages with changeType none', () => {
+    tagPackages({ ...oneTagBumpInfo, calculatedChangeTypes: { foo: 'none' } }, { path: '', gitTags: false, tag: '' });
     expect(gitFailFast).not.toHaveBeenCalled();
   });
 
-  it('does not create git tag for "latest" dist tag', () => {
-    tagPackages(emptyBumpInfo as BumpInfo, { path: '', gitTags: true, tag: 'latest' });
+  it('does not create overall git tag for empty dist tag', () => {
+    tagPackages(emptyBumpInfo, { path: '', gitTags: true, tag: '' });
     expect(gitFailFast).not.toHaveBeenCalled();
   });
 
-  it('creates git tag for non-"latest" dist tag', () => {
-    tagPackages(emptyBumpInfo as BumpInfo, { path: '', gitTags: true, tag: 'abc' });
+  it('does not create overall git tag for "latest" dist tag', () => {
+    tagPackages(emptyBumpInfo, { path: '', gitTags: true, tag: 'latest' });
+    expect(gitFailFast).not.toHaveBeenCalled();
+  });
+
+  it('creates overall git tag for non-"latest" dist tag', () => {
+    tagPackages(emptyBumpInfo, { path: '', gitTags: true, tag: 'abc' });
     expect(gitFailFast).toBeCalledTimes(1);
     expect(gitFailFast).toHaveBeenCalledWith(...createTagParameters('abc', ''));
   });

src/bump/gatherBumpInfo.ts

@@ -28,7 +28,6 @@ export function gatherBumpInfo(options: BeachballOptions, packageInfos: PackageI
     packageGroups: getPackageGroups(packageInfos, options.path, options.groups),
     changeFileChangeInfos: changes,
     modifiedPackages: new Set<string>(),
-    newPackages: new Set<string>(),
     scopedPackages: new Set(getScopedPackages(options, packageInfos)),
     dependentChangedBy: {},
   };

src/bump/getDependentsForPackages.ts

@@ -12,7 +12,7 @@ export function getDependentsForPackages(
 ): PackageDependents {
   const { packageInfos, scopedPackages } = bumpInfo;
 
-  const dependents: PackageDependents = {};
+  const dependents: { [pkgName: string]: string[] } = {};
 
   for (const [pkgName, info] of Object.entries(packageInfos)) {
     if (!scopedPackages.has(pkgName)) {

src/commands/publish.ts

@@ -7,6 +7,7 @@ import { bumpAndPush } from '../publish/bumpAndPush';
 import { publishToRegistry } from '../publish/publishToRegistry';
 import { getNewPackages } from '../publish/getNewPackages';
 import { getPackageInfos } from '../monorepo/getPackageInfos';
+import { PublishBumpInfo } from '../types/BumpInfo';
 
 export async function publish(options: BeachballOptions): Promise<void> {
   console.log('\nPreparing to publish');
@@ -57,14 +58,21 @@ export async function publish(options: BeachballOptions): Promise<void> {
   gitFailFast(['checkout', '-b', publishBranch], { cwd });
 
   console.log(`\nGathering info ${options.bump ? 'to bump versions' : 'about versions and changes'}`);
-  const bumpInfo = gatherBumpInfo(options, oldPackageInfos);
+  const bumpInfo: PublishBumpInfo = gatherBumpInfo(options, oldPackageInfos);
+
   if (options.new) {
     // Publish newly created packages even if they don't have change files
     // (this is unlikely unless the packages were pushed without a PR that runs "beachball check")
-    bumpInfo.newPackages = new Set<string>(await getNewPackages(bumpInfo, options));
+    console.log(
+      '\nFetching all unmodified packages from the registry to check if there are any ' +
+        "newly-added packages that didn't have a change file...\n" +
+        '(If your PR build runs `beachball check`, it should be safe to disable this step by ' +
+        'removing `new: true` from your config or removing `--new` from your publish command.)'
+    );
+    bumpInfo.newPackages = await getNewPackages(bumpInfo, options);
   }
 
-  // Step 1. Bump + npm publish
+  // Step 1. Bump on disk + npm publish
   // npm / yarn publish
   if (options.publish) {
     console.log('\nBumping versions and publishing to npm');
@@ -75,7 +83,8 @@ export async function publish(options: BeachballOptions): Promise<void> {
   }
 
   // Step 2.
-  // - reset, fetch latest from origin/master (to ensure less chance of conflict), then bump again + commit
+  // - reset, fetch latest from origin/master (to ensure less chance of conflict),
+  //   then bump on disk again + commit
   if (options.bump && branch && options.push) {
     // this does its own section logging
     await bumpAndPush(bumpInfo, publishBranch, options);

src/publish/getPackagesToPublish.ts

@@ -1,5 +1,5 @@
 import { formatList } from '../logging/format';
-import { BumpInfo } from '../types/BumpInfo';
+import { PublishBumpInfo } from '../types/BumpInfo';
 import { toposortPackages } from './toposortPackages';
 
 /**
@@ -10,10 +10,10 @@ import { toposortPackages } from './toposortPackages';
  * based on the dependency graph to ensure they're published in the correct order, and any
  * new/modified packages that will be skipped (and why) are logged to the console.
  */
-export function getPackagesToPublish(bumpInfo: BumpInfo, validationMode?: boolean): string[] {
-  const { modifiedPackages, newPackages, packageInfos } = bumpInfo;
+export function getPackagesToPublish(bumpInfo: PublishBumpInfo, validationMode?: boolean): string[] {
+  const { modifiedPackages, newPackages, packageInfos, calculatedChangeTypes, scopedPackages } = bumpInfo;
 
-  let packages = [...modifiedPackages, ...newPackages];
+  let packages = [...modifiedPackages, ...(newPackages || [])];
   if (!validationMode) {
     // skip this step when called from `validate` since it's not needed and might be slow
     packages = toposortPackages(packages, packageInfos);
@@ -22,15 +22,15 @@ export function getPackagesToPublish(bumpInfo: BumpInfo, validationMode?: boolea
   const skippedPackageReasons: string[] = [];
 
   for (const pkg of packages) {
-    const packageInfo = bumpInfo.packageInfos[pkg];
-    const changeType = bumpInfo.calculatedChangeTypes[pkg];
+    const packageInfo = packageInfos[pkg];
+    const changeType = calculatedChangeTypes[pkg];
 
     let skipReason = '';
     if (changeType === 'none') {
       skipReason = 'has change type none';
     } else if (packageInfo.private) {
       skipReason = 'is private';
-    } else if (!bumpInfo.scopedPackages.has(pkg)) {
+    } else if (!scopedPackages.has(pkg)) {
       skipReason = 'is out-of-scope';
     }
 

src/publish/publishToRegistry.ts

@@ -1,6 +1,6 @@
 import _ from 'lodash';
 import { performBump } from '../bump/performBump';
-import { BumpInfo } from '../types/BumpInfo';
+import { PublishBumpInfo } from '../types/BumpInfo';
 import { BeachballOptions } from '../types/BeachballOptions';
 import { packagePublish } from '../packageManager/packagePublish';
 import { validatePackageVersions } from './validatePackageVersions';
@@ -14,7 +14,7 @@ import { callHook } from '../bump/callHook';
  * Publish all the bumped packages to the registry.
  * This will bump packages on the filesystem first if `options.bump` is true.
  */
-export async function publishToRegistry(originalBumpInfo: BumpInfo, options: BeachballOptions): Promise<void> {
+export async function publishToRegistry(originalBumpInfo: PublishBumpInfo, options: BeachballOptions): Promise<void> {
   const bumpInfo = _.cloneDeep(originalBumpInfo);
 
   if (options.bump) {
@@ -37,7 +37,8 @@ export async function publishToRegistry(originalBumpInfo: BumpInfo, options: Bea
     process.exit(1);
   }
 
-  // performing publishConfig and workspace version overrides requires this procedure to ONLY be run right before npm publish, but NOT in the git push
+  // performing publishConfig and workspace version overrides requires this procedure to
+  // ONLY be run right before npm publish, but NOT in the git push
   performPublishOverrides(packagesToPublish, bumpInfo.packageInfos);
 
   // if there is a prepublish hook perform a prepublish pass, calling the routine on each package

src/publish/tagPackages.ts

@@ -1,7 +1,8 @@
-import { BumpInfo } from '../types/BumpInfo';
+import { PublishBumpInfo } from '../types/BumpInfo';
 import { generateTag } from '../git/generateTag';
 import { gitFailFast } from 'workspace-tools';
 import { BeachballOptions } from '../types/BeachballOptions';
+import { DeepReadonly } from '../types/DeepReadonly';
 
 function createTag(tag: string, cwd: string): void {
   gitFailFast(['tag', '-a', '-f', tag, '-m', tag], { cwd });
@@ -12,13 +13,19 @@ function createTag(tag: string, cwd: string): void {
  * Also, if git tags aren't disabled for the repo and the overall dist-tag (`options.tag`) has a
  * non-default value (not "latest"), create a git tag for the dist-tag.
  */
-export function tagPackages(bumpInfo: BumpInfo, options: Pick<BeachballOptions, 'gitTags' | 'path' | 'tag'>): void {
+export function tagPackages(
+  bumpInfo: Pick<
+    DeepReadonly<PublishBumpInfo>,
+    'modifiedPackages' | 'newPackages' | 'packageInfos' | 'calculatedChangeTypes'
+  >,
+  options: Pick<BeachballOptions, 'gitTags' | 'path' | 'tag'>
+): void {
   const { gitTags, tag: distTag, path: cwd } = options;
-  const { modifiedPackages, newPackages } = bumpInfo;
+  const { modifiedPackages, newPackages, packageInfos, calculatedChangeTypes } = bumpInfo;
 
-  for (const pkg of [...modifiedPackages, ...newPackages]) {
-    const packageInfo = bumpInfo.packageInfos[pkg];
-    const changeType = bumpInfo.calculatedChangeTypes[pkg];
+  for (const pkg of [...modifiedPackages, ...(newPackages || [])]) {
+    const packageInfo = packageInfos[pkg];
+    const changeType = calculatedChangeTypes[pkg];
     // Do not tag change type of "none", private packages, or packages opting out of tagging
     if (changeType === 'none' || packageInfo.private || !packageInfo.combinedOptions.gitTags) {
       continue;

src/types/BumpInfo.ts

@@ -21,9 +21,6 @@ export type BumpInfo = {
   /** Set of packages that had been modified */
   modifiedPackages: Set<string>;
 
-  /** Set of new packages detected in this info */
-  newPackages: Set<string>;
-
   /** Map from package name to its internal dependency names that were bumped. */
   dependentChangedBy: { [pkgName: string]: Set<string> };
 
@@ -32,4 +29,16 @@ export type BumpInfo = {
 };
 
 /** Dependents cache (child points to parents): if A depends on B, then `{ B: [A] }` */
-export type PackageDependents = { [pkgName: string]: string[] };
+export type PackageDependents = { readonly [pkgName: string]: ReadonlyArray<string> };
+
+/**
+ * Bump info with additional property set/used only during publishing (not while calculating
+ * packages to bump).
+ */
+export type PublishBumpInfo = BumpInfo & {
+  /**
+   * Set of packages detected in this info which weren't previously published and didn't have
+   * change files. (Only populated if `options.new` is set.)
+   */
+  newPackages?: ReadonlyArray<string>;
+};