From aadc2e0d265cde7dfdfaa7666e79e63738016b30 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 3 Jan 2025 14:52:31 -0800 Subject: [PATCH 01/15] Setup tagging for tree's "fail" --- .../src/commands/generate/assertTags.ts | 191 +++++++++++------- build-tools/packages/build-cli/src/config.ts | 21 +- packages/dds/tree/flub.config.cjs | 13 ++ 3 files changed, 148 insertions(+), 77 deletions(-) create mode 100644 packages/dds/tree/flub.config.cjs diff --git a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts index e47933b7060b..79a9628e140e 100644 --- a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts +++ b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts @@ -3,12 +3,11 @@ * Licensed under the MIT License. */ -import { strict as assert } from "node:assert"; import fs from "node:fs"; import path from "node:path"; import { Package } from "@fluidframework/build-tools"; import { PackageCommand } from "../../BasePackageCommand.js"; -import { PackageKind } from "../../filter.js"; +import { PackageKind, type PackageWithKind } from "../../filter.js"; import { Flags } from "@oclif/core"; import { @@ -20,13 +19,35 @@ import { StringLiteral, SyntaxKind, } from "ts-morph"; +import { getFlubConfig } from "../../config.js"; -const shortCodes = new Map(); -const newAssetFiles = new Set(); -const codeToMsgMap = new Map(); -let maxShortCode = -1; +/** + * Key is the name of the assert function. + * Value is the index of the augment to tag. + * @remarks + * The function names are not handled in a scoped/qualified way, so any function imported or declared with that name will have tagging applied. + * See also {@link AssertTaggingConfig.assertionFunctions}. + */ +type AssertionFunctions = ReadonlyMap; + +const defaultAssertionFunctions: AssertionFunctions = new Map([["assert", 1]]); + +/** + * Data aggregated about a collection of packages. + */ +interface CollectedData { + readonly shortCodes: Map; + readonly codeToMsgMap: Map; + maxShortCode: number; +} -const defaultAssertionFunctions: ReadonlyMap = new Map([["assert", 1]]); +/** + * Data about a specific package. + */ +interface PackageData { + readonly newAssetFiles: ReadonlySet; + readonly assertionFunctions: AssertionFunctions; +} export class TagAssertsCommand extends PackageCommand { static readonly summary = @@ -47,9 +68,7 @@ export class TagAssertsCommand extends PackageCommand protected defaultSelection = undefined; - private assertionFunctions: ReadonlyMap | undefined; - private readonly errors: string[] = []; - + // TODO: just use per package config and default (inherited) filtering logic. protected async selectAndFilterPackages(): Promise { await super.selectAndFilterPackages(); @@ -59,11 +78,6 @@ export class TagAssertsCommand extends PackageCommand ? undefined : assertTagging?.enabledPaths; - this.assertionFunctions = - assertTagging?.assertionFunctions === undefined - ? defaultAssertionFunctions - : new Map(Object.entries(assertTagging.assertionFunctions)); - // Further filter packages based on the path regex const before = this.filteredPackages?.length ?? 0; this.filteredPackages = this.filteredPackages?.filter((pkg) => { @@ -97,71 +111,105 @@ export class TagAssertsCommand extends PackageCommand } } - protected async processPackage( + // This should not be used due to processPackages being overridden instead. + protected override processPackage( pkg: TPkg, kind: PackageKind, ): Promise { - const tsconfigPath = await this.getTsConfigPath(pkg); - this.collectAssertData(tsconfigPath); + throw new Error("Method not implemented."); } - public async run(): Promise { - // Calls processPackage on all packages to collect assert data. - await super.run(); + protected override async processPackages(packages: PackageWithKind[]): Promise { + const errors: string[] = []; - // Tag asserts based on earlier collected data. - this.tagAsserts(true); - } + const collected: CollectedData = { + shortCodes: new Map(), + codeToMsgMap: new Map(), + maxShortCode: -1, + }; + + const dataMap = new Map(); + + for (const pkg of packages) { + // Package configuration: + const tsconfigPath = await this.getTsConfigPath(pkg); + const packageConfig = getFlubConfig(pkg.directory).assertTagging; + const assertionFunctions: AssertionFunctions = + packageConfig?.assertionFunctions === undefined + ? defaultAssertionFunctions + : new Map(Object.entries(packageConfig.assertionFunctions)); + + // load the project based on the tsconfig + const project = new Project({ + skipFileDependencyResolution: true, + tsConfigFilePath: tsconfigPath, + }); + + const newAssetFiles = this.collectAssertData( + project, + assertionFunctions, + collected, + errors, + ); + dataMap.set(pkg, { assertionFunctions, newAssetFiles }); + } - private collectAssertData(tsconfigPath: string): void { - // TODO: this can probably be removed now - if (tsconfigPath.includes("test")) { - return; + if (errors.length !== 0) { + return errors; } - // load the project based on the tsconfig - const project = new Project({ - skipFileDependencyResolution: true, - tsConfigFilePath: tsconfigPath, - }); + for (const [pkg, data] of dataMap) { + errors.push(...this.tagAsserts(collected, data)); + } + + writeShortCodeMappingFile(collected.codeToMsgMap); + + return errors; + } + private collectAssertData( + project: Project, + assertionFunctions: AssertionFunctions, + collected: CollectedData, + errors: string[], + ): Set { const templateErrors: Node[] = []; const otherErrors: Node[] = []; + const newAssetFiles = new Set(); // walk all the files in the project for (const sourceFile of project.getSourceFiles()) { // walk the assert message params in the file - assert(this.assertionFunctions !== undefined, "No assert functions are defined!"); - for (const msg of getAssertMessageParams(sourceFile, this.assertionFunctions)) { + for (const msg of getAssertMessageParams(sourceFile, assertionFunctions)) { const nodeKind = msg.getKind(); switch (nodeKind) { // If it's a number, validate it's a shortcode case SyntaxKind.NumericLiteral: { const numLit = msg as NumericLiteral; if (!numLit.getText().startsWith("0x")) { - this.errors.push( + errors.push( `Shortcodes must be provided by automation and be in hex format: ${numLit.getText()}\n\t${getCallsiteString( numLit, )}`, ); - return; + return newAssetFiles; } const numLitValue = numLit.getLiteralValue(); - if (shortCodes.has(numLitValue)) { + if (collected.shortCodes.has(numLitValue)) { // if we find two usages of the same short code then fail - this.errors.push( + errors.push( `Duplicate shortcode 0x${numLitValue.toString( 16, )} detected\n\t${getCallsiteString( // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - shortCodes.get(numLitValue)!, + collected.shortCodes.get(numLitValue)!, )}\n\t${getCallsiteString(numLit)}`, ); - return; + return newAssetFiles; } - shortCodes.set(numLitValue, numLit); + collected.shortCodes.set(numLitValue, numLit); // calculate the maximun short code to ensure we don't duplicate - maxShortCode = Math.max(numLitValue, maxShortCode); + collected.maxShortCode = Math.max(numLitValue, collected.maxShortCode); // If comment already exists, extract it for the mapping file const comments = msg.getTrailingCommentRanges(); @@ -191,7 +239,7 @@ export class TagAssertsCommand extends PackageCommand originalErrorText.length - 1, ); } - codeToMsgMap.set(numLit.getText(), originalErrorText); + collected.codeToMsgMap.set(numLit.getText(), originalErrorText); } break; } @@ -239,10 +287,16 @@ export class TagAssertsCommand extends PackageCommand if (errorMessages.length > 0) { this.error(errorMessages.join("\n\n"), { exit: 1 }); } + + return newAssetFiles; } - // TODO: the resolve = true may be safe to remove since we always want to resolve when running this command - private tagAsserts(resolve: true): void { + /** + * Updates source files, adding new asserts to `collected`. + * + * @return array of error strings. + */ + private tagAsserts(collected: CollectedData, packageData: PackageData): string[] { const errors: string[] = []; // eslint-disable-next-line unicorn/consistent-function-scoping @@ -256,44 +310,28 @@ export class TagAssertsCommand extends PackageCommand } // go through all the newly collected asserts and add short codes - for (const s of newAssetFiles) { + for (const s of packageData.newAssetFiles) { // another policy may have changed the file, so reload it s.refreshFromFileSystemSync(); - assert(this.assertionFunctions !== undefined, "No assert functions are defined!"); - for (const msg of getAssertMessageParams(s, this.assertionFunctions)) { + for (const msg of getAssertMessageParams(s, packageData.assertionFunctions)) { // here we only want to look at those messages that are strings, // as we validated existing short codes above if (isStringLiteral(msg)) { - // resolve === fix - if (resolve) { - // for now we don't care about filling gaps, but possible - const shortCode = ++maxShortCode; - shortCodes.set(shortCode, msg); - const text = msg.getLiteralText(); - const shortCodeStr = `0x${shortCode.toString(16).padStart(3, "0")}`; - // replace the message with shortcode, and put the message in a comment - msg.replaceWithText(`${shortCodeStr} /* ${text} */`); - codeToMsgMap.set(shortCodeStr, text); - } else { - // TODO: if we are not in resolve mode we - // allow messages that are not short code. this seems like the right - // behavior for main. we may want to enforce shortcodes in release branches in the future - // errors.push(`no assert shortcode: ${getCallsiteString(msg)}`); - break; - } + // for now we don't care about filling gaps, but possible + const shortCode = ++collected.maxShortCode; + collected.shortCodes.set(shortCode, msg); + const text = msg.getLiteralText(); + const shortCodeStr = `0x${shortCode.toString(16).padStart(3, "0")}`; + // replace the message with shortcode, and put the message in a comment + msg.replaceWithText(`${shortCodeStr} /* ${text} */`); + collected.codeToMsgMap.set(shortCodeStr, text); } } - if (resolve) { - s.saveSync(); - } - } - if (resolve) { - writeShortCodeMappingFile(); - } - if (errors.length > 0) { - this.error(errors.join("\n"), { exit: 1 }); + s.saveSync(); } + + return errors; } private async getTsConfigPath(pkg: Package): Promise { @@ -343,7 +381,7 @@ function getAssertMessageParams( return messageArgs; } -function writeShortCodeMappingFile(): void { +function writeShortCodeMappingFile(codeToMsgMap: Map): void { // eslint-disable-next-line unicorn/prefer-spread, @typescript-eslint/no-unsafe-assignment const mapContents = Array.from(codeToMsgMap.entries()) .sort() @@ -355,6 +393,7 @@ function writeShortCodeMappingFile(): void { return accum; // eslint-disable-next-line @typescript-eslint/no-explicit-any }, {} as any); + // TODO: this should come from config. const targetFolder = "packages/runtime/test-runtime-utils/src"; if (!fs.existsSync(targetFolder)) { diff --git a/build-tools/packages/build-cli/src/config.ts b/build-tools/packages/build-cli/src/config.ts index 29f1571fb569..fe2ddde30d10 100644 --- a/build-tools/packages/build-cli/src/config.ts +++ b/build-tools/packages/build-cli/src/config.ts @@ -40,6 +40,11 @@ export interface FlubConfig { /** * Configuration for assert tagging. + * @remarks + * Some of this applies to the root where flub is being run, + * and some of it applies to the specific package being processed. + * @privateRemarks + * It seems like having each package have its own configuration would be simpler. */ assertTagging?: AssertTaggingConfig; @@ -212,12 +217,26 @@ export interface PolicyConfig { publicPackageRequirements?: PackageRequirements; } +/** + * Used by {@link TagAssertsCommand}. + */ export interface AssertTaggingConfig { - assertionFunctions: { [functionName: string]: number }; + /** + * Property key is the name of the assert function. + * Property value is the index of the augment to tag. + * @remarks + * The function names are not handled in a scoped/qualified way, so any function imported or declared with that name will have tagging applied. + * See also {@link AssertTaggingConfig.assertionFunctions} + * + * This applies to the package it is in. + */ + assertionFunctions?: { [functionName: string]: number }; /** * An array of paths under which assert tagging applies to. If this setting is provided, only packages whose paths * match the regular expressions in this setting will be assert-tagged. + * + * This is used from the root where flub is run. */ enabledPaths?: RegExp[]; } diff --git a/packages/dds/tree/flub.config.cjs b/packages/dds/tree/flub.config.cjs new file mode 100644 index 000000000000..4cbc57549ce9 --- /dev/null +++ b/packages/dds/tree/flub.config.cjs @@ -0,0 +1,13 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +"use strict"; + +module.exports = { + "assertTagging": { + "assert": 1, + "fail": 0, + }, +}; From c16f9874e3ab2667c43d40bf40fd49c3c3ca8b53 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 3 Jan 2025 15:29:35 -0800 Subject: [PATCH 02/15] Fix fail tagging --- .../packages/build-cli/api-report/build-cli.api.md | 5 ++--- .../build-cli/src/commands/generate/assertTags.ts | 7 ++++--- build-tools/packages/build-cli/src/config.ts | 2 +- packages/dds/test-dds-utils/flub.config.cjs | 12 ++++++++++++ packages/dds/tree/flub.config.cjs | 8 +++++--- packages/runtime/test-runtime-utils/flub.config.cjs | 12 ++++++++++++ packages/test/flub.config.cjs | 12 ++++++++++++ 7 files changed, 48 insertions(+), 10 deletions(-) create mode 100644 packages/dds/test-dds-utils/flub.config.cjs create mode 100644 packages/runtime/test-runtime-utils/flub.config.cjs create mode 100644 packages/test/flub.config.cjs diff --git a/build-tools/packages/build-cli/api-report/build-cli.api.md b/build-tools/packages/build-cli/api-report/build-cli.api.md index f9c94ef77b40..6c36aa973253 100644 --- a/build-tools/packages/build-cli/api-report/build-cli.api.md +++ b/build-tools/packages/build-cli/api-report/build-cli.api.md @@ -8,10 +8,9 @@ import { InterdependencyRange } from '@fluid-tools/version-tools'; import { run } from '@oclif/core'; import { VersionBumpType } from '@fluid-tools/version-tools'; -// @public (undocumented) +// @public export interface AssertTaggingConfig { - // (undocumented) - assertionFunctions: { + assertionFunctions?: { [functionName: string]: number; }; enabledPaths?: RegExp[]; diff --git a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts index 79a9628e140e..0ed72462ae89 100644 --- a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts +++ b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts @@ -112,7 +112,7 @@ export class TagAssertsCommand extends PackageCommand } // This should not be used due to processPackages being overridden instead. - protected override processPackage( + protected override async processPackage( pkg: TPkg, kind: PackageKind, ): Promise { @@ -132,6 +132,7 @@ export class TagAssertsCommand extends PackageCommand for (const pkg of packages) { // Package configuration: + // eslint-disable-next-line no-await-in-loop const tsconfigPath = await this.getTsConfigPath(pkg); const packageConfig = getFlubConfig(pkg.directory).assertTagging; const assertionFunctions: AssertionFunctions = @@ -154,7 +155,7 @@ export class TagAssertsCommand extends PackageCommand dataMap.set(pkg, { assertionFunctions, newAssetFiles }); } - if (errors.length !== 0) { + if (errors.length > 0) { return errors; } @@ -294,7 +295,7 @@ export class TagAssertsCommand extends PackageCommand /** * Updates source files, adding new asserts to `collected`. * - * @return array of error strings. + * @returns array of error strings. */ private tagAsserts(collected: CollectedData, packageData: PackageData): string[] { const errors: string[] = []; diff --git a/build-tools/packages/build-cli/src/config.ts b/build-tools/packages/build-cli/src/config.ts index fe2ddde30d10..2b6a607b2e05 100644 --- a/build-tools/packages/build-cli/src/config.ts +++ b/build-tools/packages/build-cli/src/config.ts @@ -218,7 +218,7 @@ export interface PolicyConfig { } /** - * Used by {@link TagAssertsCommand}. + * Used by `TagAssertsCommand`. */ export interface AssertTaggingConfig { /** diff --git a/packages/dds/test-dds-utils/flub.config.cjs b/packages/dds/test-dds-utils/flub.config.cjs new file mode 100644 index 000000000000..6808d55b90a5 --- /dev/null +++ b/packages/dds/test-dds-utils/flub.config.cjs @@ -0,0 +1,12 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +"use strict"; + +module.exports = { + assertTagging: { + assertionFunctions: {}, + }, +}; diff --git a/packages/dds/tree/flub.config.cjs b/packages/dds/tree/flub.config.cjs index 4cbc57549ce9..a1b394114022 100644 --- a/packages/dds/tree/flub.config.cjs +++ b/packages/dds/tree/flub.config.cjs @@ -6,8 +6,10 @@ "use strict"; module.exports = { - "assertTagging": { - "assert": 1, - "fail": 0, + assertTagging: { + assertionFunctions: { + "assert": 1, + "fail": 0, + }, }, }; diff --git a/packages/runtime/test-runtime-utils/flub.config.cjs b/packages/runtime/test-runtime-utils/flub.config.cjs new file mode 100644 index 000000000000..6808d55b90a5 --- /dev/null +++ b/packages/runtime/test-runtime-utils/flub.config.cjs @@ -0,0 +1,12 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +"use strict"; + +module.exports = { + assertTagging: { + assertionFunctions: {}, + }, +}; diff --git a/packages/test/flub.config.cjs b/packages/test/flub.config.cjs new file mode 100644 index 000000000000..6808d55b90a5 --- /dev/null +++ b/packages/test/flub.config.cjs @@ -0,0 +1,12 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +"use strict"; + +module.exports = { + assertTagging: { + assertionFunctions: {}, + }, +}; From 5e1ce1f8d211b7cb2999e63ccdcbdb22d8c9e9f8 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 3 Jan 2025 15:34:31 -0800 Subject: [PATCH 03/15] remove fail templates --- .../feature-libraries/sequence-field/sequenceFieldCodecV1.ts | 2 +- .../feature-libraries/sequence-field/sequenceFieldCodecV2.ts | 2 +- packages/dds/tree/src/simple-tree/api/treeNodeApi.ts | 2 +- packages/dds/tree/src/simple-tree/toMapTree.ts | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/dds/tree/src/feature-libraries/sequence-field/sequenceFieldCodecV1.ts b/packages/dds/tree/src/feature-libraries/sequence-field/sequenceFieldCodecV1.ts index 8117ed37ffe4..5a4fbbff1af7 100644 --- a/packages/dds/tree/src/feature-libraries/sequence-field/sequenceFieldCodecV1.ts +++ b/packages/dds/tree/src/feature-libraries/sequence-field/sequenceFieldCodecV1.ts @@ -145,7 +145,7 @@ export function makeV1Codec( context, ); case NoopMarkType: - fail(`Mark type: ${type} should not be encoded.`); + fail("Mark type: NoopMarkType should not be encoded."); default: unreachableCase(type); } diff --git a/packages/dds/tree/src/feature-libraries/sequence-field/sequenceFieldCodecV2.ts b/packages/dds/tree/src/feature-libraries/sequence-field/sequenceFieldCodecV2.ts index daa3d4a19403..1ed84c23ec73 100644 --- a/packages/dds/tree/src/feature-libraries/sequence-field/sequenceFieldCodecV2.ts +++ b/packages/dds/tree/src/feature-libraries/sequence-field/sequenceFieldCodecV2.ts @@ -132,7 +132,7 @@ export function makeV2CodecHelpers( context, ); case NoopMarkType: - fail(`Mark type: ${type} should not be encoded.`); + fail("Mark type: NoopMarkType should not be encoded."); default: unreachableCase(type); } diff --git a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts index fb71ba8a3e35..81ef0357e76d 100644 --- a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts +++ b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts @@ -174,7 +174,7 @@ export const treeNodeApi: TreeNodeApi = { changedFields, (field) => nodeSchema.storedKeyToPropertyKey.get(field) ?? - fail(`Could not find stored key '${field}' in schema.`), + fail("Could not find stored key in schema."), ), ); listener({ changedProperties }); diff --git a/packages/dds/tree/src/simple-tree/toMapTree.ts b/packages/dds/tree/src/simple-tree/toMapTree.ts index 84ba65a2a63d..f5ffa9feddf6 100644 --- a/packages/dds/tree/src/simple-tree/toMapTree.ts +++ b/packages/dds/tree/src/simple-tree/toMapTree.ts @@ -196,7 +196,7 @@ function nodeDataToMapTree( result = objectToMapTree(data, schema); break; default: - fail(`Unrecognized schema kind: ${schema.kind}.`); + fail("Unrecognized schema kind"); } return result; From 89ce236e56abe0d7605d0eccdf09303dc6146c6b Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 3 Jan 2025 15:38:12 -0800 Subject: [PATCH 04/15] Update fail to allow tagged messages --- packages/dds/tree/src/util/utils.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/dds/tree/src/util/utils.ts b/packages/dds/tree/src/util/utils.ts index 476f02b4b471..23805c77cef3 100644 --- a/packages/dds/tree/src/util/utils.ts +++ b/packages/dds/tree/src/util/utils.ts @@ -48,9 +48,14 @@ export function asMutable(readonly: T): Mutable { export const clone = structuredClone; /** + * Throw an error with a constant message. + * @remarks + * Works like {@link @fluidframework/core-utils/internal#assert}. */ -export function fail(message: string): never { - throw new Error(message); +export function fail(message: string | number): never { + throw new Error( + typeof message === "number" ? `0x${message.toString(16).padStart(3, "0")}` : message, + ); } /** From 5c06f20cacaefbf8ab2294b09aba31b782396cf3 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Tue, 7 Jan 2025 10:33:44 -0800 Subject: [PATCH 05/15] Work on comments --- .../src/commands/generate/assertTags.ts | 42 ++++++++++++++----- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts index 0ed72462ae89..5757da983937 100644 --- a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts +++ b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts @@ -68,8 +68,35 @@ export class TagAssertsCommand extends PackageCommand protected defaultSelection = undefined; - // TODO: just use per package config and default (inherited) filtering logic. - protected async selectAndFilterPackages(): Promise { + // TODO: + // Refactor regex based filtering logic. + // Consider doing one the below instead of applying this filtering here: + // a. Add regex filter CLI option to PackageCommand. + // b. Have packages which want to opt out do so in their own configuration. + // c. Refactor TagAssertsCommand so it doesn't have to rely on this override and undocumented implementation details to do this filtering + // (Ex: move the logic into an early continue in processPackages) + // d. Refactor PackageCommand so having subclasses customize filtering is well supported + // (ex: allow the subclass to provide a filtering predicate, perhaps via the constructor or an a method explicitly documented to be used for overriding which normally just returns true). + // + // The current approach isn't ideal from a readability or maintainability perspective. + // 1. selectAndFilterPackages is undocumented had relies on side effects. + // To override it correctly the the subclass must know and depend on many undocumented details of the base class (like that this method sets filteredPackages, that its ok for it to modify filteredPackages). + // This makes the base class fragile: refactoring it to work slightly differently (like cache data derived from the set of filteredPackages after they are computed) could break things. + // 2. Data flow is hard to follow. This method does not have inputs or outputs declared in its signature, and the values it reads from the class arn't readonly so its hard to know what is initialized when + // and which values are functions of which other values. + // 3. The division of responsibility here is odd. Generally the user of a PackageCommand selects which packages to apply it to on the command line. + // Currently this is done by passing --all (as specified in the script that invokes this command), + // and a separate regex in a config. + // This extra config even more confusing since typically this kind of configuration is per package, + // but in this case the config from one package is applying to multiple release groups based on the working directory the command is run in. + // Normally configuration in a package applies to the package, and sometimes configuration for a release group lives at its root. + // In this case configuration spanning multiple release groups lives in the root of one of them but uses the same file we would use for per package configuration which makes it hard to understand. + // It seems like it would be more consistent with the design, and more useful generally, that if additional package filtering functionality is needed (ex: regex based filtering) that it + // be added to PackageCommand's package filtering CLI options so all PackageCommands could use it. + // This would remove the need to expose so many internals (like this method, filteredPackages, etc) of PackageCommand as "protected" + // improved testability (since this logic could reside in the more testable selectAndFilterPackages free function) and keep the per package configuration files actually per package + // while putting the cross release group configuration (--all and the regex) in the same place. + protected override async selectAndFilterPackages(): Promise { await super.selectAndFilterPackages(); const context = await this.getContext(); @@ -111,7 +138,7 @@ export class TagAssertsCommand extends PackageCommand } } - // This should not be used due to processPackages being overridden instead. + // This should not be called due to processPackages being overridden instead. protected override async processPackage( pkg: TPkg, kind: PackageKind, @@ -349,13 +376,6 @@ function getCallsiteString(msg: Node): string { return `${msg.getSourceFile().getFilePath()}:${msg.getStartLineNumber()}`; } -/** - * Map from assertion function name to the index of its message argument. - * - * TODO: - * This should be moved into a configuration file. - */ - /** * Given a source file, this function will look for all assert functions contained in it and return the message parameters. * This includes both functions named "assert" and ones named "fail" @@ -394,7 +414,7 @@ function writeShortCodeMappingFile(codeToMsgMap: Map): void { return accum; // eslint-disable-next-line @typescript-eslint/no-explicit-any }, {} as any); - // TODO: this should come from config. + // TODO: this should prabably come from configuration (if each package can have their own) or a CLI argument. const targetFolder = "packages/runtime/test-runtime-utils/src"; if (!fs.existsSync(targetFolder)) { From e55dd41b2c0032a4659f83f609252fa46935b03b Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Tue, 7 Jan 2025 10:42:31 -0800 Subject: [PATCH 06/15] fix error aggregation and extend comment --- .../build-cli/src/commands/generate/assertTags.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts index 5757da983937..d54a3b8917b2 100644 --- a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts +++ b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts @@ -78,12 +78,17 @@ export class TagAssertsCommand extends PackageCommand // d. Refactor PackageCommand so having subclasses customize filtering is well supported // (ex: allow the subclass to provide a filtering predicate, perhaps via the constructor or an a method explicitly documented to be used for overriding which normally just returns true). // - // The current approach isn't ideal from a readability or maintainability perspective. + // The current approach isn't ideal from a readability or maintainability perspective for a few reasons: + // // 1. selectAndFilterPackages is undocumented had relies on side effects. // To override it correctly the the subclass must know and depend on many undocumented details of the base class (like that this method sets filteredPackages, that its ok for it to modify filteredPackages). - // This makes the base class fragile: refactoring it to work slightly differently (like cache data derived from the set of filteredPackages after they are computed) could break things. + // This makes the base class fragile: refactoring it to work slightly differently + // (like printing the filtered packages info inside of this function instead of after it or cache data derived from the set of filteredPackages after they are computed) + // could break things. + // // 2. Data flow is hard to follow. This method does not have inputs or outputs declared in its signature, and the values it reads from the class arn't readonly so its hard to know what is initialized when // and which values are functions of which other values. + // // 3. The division of responsibility here is odd. Generally the user of a PackageCommand selects which packages to apply it to on the command line. // Currently this is done by passing --all (as specified in the script that invokes this command), // and a separate regex in a config. @@ -182,6 +187,7 @@ export class TagAssertsCommand extends PackageCommand dataMap.set(pkg, { assertionFunctions, newAssetFiles }); } + // If there are errors, avoid making code changes and just report the errors. if (errors.length > 0) { return errors; } @@ -313,7 +319,7 @@ export class TagAssertsCommand extends PackageCommand ); } if (errorMessages.length > 0) { - this.error(errorMessages.join("\n\n"), { exit: 1 }); + errors.push(errorMessages.join("\n\n")); } return newAssetFiles; From a67474c75d2c17be19ce803a629d6e3721071ed5 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Tue, 7 Jan 2025 14:57:53 -0800 Subject: [PATCH 07/15] Apply suggestions from code review Co-authored-by: Tyler Butler --- .../packages/build-cli/src/commands/generate/assertTags.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts index d54a3b8917b2..8fc0b0bfb47a 100644 --- a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts +++ b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts @@ -80,13 +80,13 @@ export class TagAssertsCommand extends PackageCommand // // The current approach isn't ideal from a readability or maintainability perspective for a few reasons: // - // 1. selectAndFilterPackages is undocumented had relies on side effects. + // 1. selectAndFilterPackages is undocumented had relied on side effects. // To override it correctly the the subclass must know and depend on many undocumented details of the base class (like that this method sets filteredPackages, that its ok for it to modify filteredPackages). // This makes the base class fragile: refactoring it to work slightly differently // (like printing the filtered packages info inside of this function instead of after it or cache data derived from the set of filteredPackages after they are computed) // could break things. // - // 2. Data flow is hard to follow. This method does not have inputs or outputs declared in its signature, and the values it reads from the class arn't readonly so its hard to know what is initialized when + // 2. Data flow is hard to follow. This method does not have inputs or outputs declared in its signature, and the values it reads from the class aren't readonly so it's hard to know what is initialized when // and which values are functions of which other values. // // 3. The division of responsibility here is odd. Generally the user of a PackageCommand selects which packages to apply it to on the command line. @@ -420,7 +420,7 @@ function writeShortCodeMappingFile(codeToMsgMap: Map): void { return accum; // eslint-disable-next-line @typescript-eslint/no-explicit-any }, {} as any); - // TODO: this should prabably come from configuration (if each package can have their own) or a CLI argument. + // TODO: this should probably come from configuration (if each package can have their own) or a CLI argument. const targetFolder = "packages/runtime/test-runtime-utils/src"; if (!fs.existsSync(targetFolder)) { From 2c8fe5c15b013f0e21c452b12599dc23625109c6 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Tue, 7 Jan 2025 15:19:00 -0800 Subject: [PATCH 08/15] workaround assert tagging to make fail call assert --- packages/dds/tree/src/util/utils.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/dds/tree/src/util/utils.ts b/packages/dds/tree/src/util/utils.ts index 23805c77cef3..1c3dc50cd3b2 100644 --- a/packages/dds/tree/src/util/utils.ts +++ b/packages/dds/tree/src/util/utils.ts @@ -53,9 +53,11 @@ export const clone = structuredClone; * Works like {@link @fluidframework/core-utils/internal#assert}. */ export function fail(message: string | number): never { - throw new Error( - typeof message === "number" ? `0x${message.toString(16).padStart(3, "0")}` : message, - ); + // Declaring this here aliased to a different name avoids the assert tagging objecting to the usages of `assert` below. + // Since users of `fail` do the assert message tagging instead, suppressing tagging errors here makes sense. + const assertNoTag: (condition: boolean, message: string | number) => asserts condition = + assert; + assertNoTag(false, message); } /** From 137a6c2c5fe1532f262274ecb7f313a968f59372 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Tue, 7 Jan 2025 15:20:43 -0800 Subject: [PATCH 09/15] Fix newAssetFiles typo --- .../src/commands/generate/assertTags.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts index 8fc0b0bfb47a..903929de8578 100644 --- a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts +++ b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts @@ -45,7 +45,7 @@ interface CollectedData { * Data about a specific package. */ interface PackageData { - readonly newAssetFiles: ReadonlySet; + readonly newAssertFiles: ReadonlySet; readonly assertionFunctions: AssertionFunctions; } @@ -178,13 +178,13 @@ export class TagAssertsCommand extends PackageCommand tsConfigFilePath: tsconfigPath, }); - const newAssetFiles = this.collectAssertData( + const newAssertFiles = this.collectAssertData( project, assertionFunctions, collected, errors, ); - dataMap.set(pkg, { assertionFunctions, newAssetFiles }); + dataMap.set(pkg, { assertionFunctions, newAssertFiles: newAssertFiles }); } // If there are errors, avoid making code changes and just report the errors. @@ -209,7 +209,7 @@ export class TagAssertsCommand extends PackageCommand ): Set { const templateErrors: Node[] = []; const otherErrors: Node[] = []; - const newAssetFiles = new Set(); + const newAssertFiles = new Set(); // walk all the files in the project for (const sourceFile of project.getSourceFiles()) { @@ -226,7 +226,7 @@ export class TagAssertsCommand extends PackageCommand numLit, )}`, ); - return newAssetFiles; + return newAssertFiles; } const numLitValue = numLit.getLiteralValue(); if (collected.shortCodes.has(numLitValue)) { @@ -239,7 +239,7 @@ export class TagAssertsCommand extends PackageCommand collected.shortCodes.get(numLitValue)!, )}\n\t${getCallsiteString(numLit)}`, ); - return newAssetFiles; + return newAssertFiles; } collected.shortCodes.set(numLitValue, numLit); // calculate the maximun short code to ensure we don't duplicate @@ -280,7 +280,7 @@ export class TagAssertsCommand extends PackageCommand // If it's a simple string literal, track the file for replacements later case SyntaxKind.StringLiteral: case SyntaxKind.NoSubstitutionTemplateLiteral: { - newAssetFiles.add(sourceFile); + newAssertFiles.add(sourceFile); break; } // Anything else isn't supported @@ -322,7 +322,7 @@ export class TagAssertsCommand extends PackageCommand errors.push(errorMessages.join("\n\n")); } - return newAssetFiles; + return newAssertFiles; } /** @@ -344,7 +344,7 @@ export class TagAssertsCommand extends PackageCommand } // go through all the newly collected asserts and add short codes - for (const s of packageData.newAssetFiles) { + for (const s of packageData.newAssertFiles) { // another policy may have changed the file, so reload it s.refreshFromFileSystemSync(); for (const msg of getAssertMessageParams(s, packageData.assertionFunctions)) { From 18ae12cecf4f86767e0a5aedc6e726bcc2a9ee8e Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Wed, 8 Jan 2025 15:39:59 -0800 Subject: [PATCH 10/15] Give assert tagging its own config --- .../build-cli/api-report/build-cli.api.md | 8 +++- .../src/commands/generate/assertTags.ts | 43 ++++++++++++++++--- build-tools/packages/build-cli/src/config.ts | 12 +----- build-tools/packages/build-cli/src/index.ts | 3 ++ fluidBuild.config.cjs | 3 -- ...ub.config.cjs => assertTagging.config.mjs} | 11 ++--- packages/dds/tree/assertTagging.config.mjs | 14 ++++++ .../assertTagging.config.mjs | 9 ++++ .../assertTagging.config.mjs} | 11 ++--- packages/test/flub.config.cjs | 12 ------ 10 files changed, 77 insertions(+), 49 deletions(-) rename packages/dds/test-dds-utils/{flub.config.cjs => assertTagging.config.mjs} (51%) create mode 100644 packages/dds/tree/assertTagging.config.mjs create mode 100644 packages/runtime/test-runtime-utils/assertTagging.config.mjs rename packages/{runtime/test-runtime-utils/flub.config.cjs => test/assertTagging.config.mjs} (51%) delete mode 100644 packages/test/flub.config.cjs diff --git a/build-tools/packages/build-cli/api-report/build-cli.api.md b/build-tools/packages/build-cli/api-report/build-cli.api.md index 6c36aa973253..392794353b26 100644 --- a/build-tools/packages/build-cli/api-report/build-cli.api.md +++ b/build-tools/packages/build-cli/api-report/build-cli.api.md @@ -10,10 +10,14 @@ import { VersionBumpType } from '@fluid-tools/version-tools'; // @public export interface AssertTaggingConfig { - assertionFunctions?: { + enabledPaths?: RegExp[]; +} + +// @public +export interface AssertTaggingPackageConfig { + assertionFunctions: { [functionName: string]: number; }; - enabledPaths?: RegExp[]; } // @public diff --git a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts index 903929de8578..819fb99044f7 100644 --- a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts +++ b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts @@ -9,7 +9,9 @@ import { Package } from "@fluidframework/build-tools"; import { PackageCommand } from "../../BasePackageCommand.js"; import { PackageKind, type PackageWithKind } from "../../filter.js"; +import assert from "node:assert"; import { Flags } from "@oclif/core"; +import { cosmiconfig } from "cosmiconfig"; import { NoSubstitutionTemplateLiteral, Node, @@ -19,14 +21,28 @@ import { StringLiteral, SyntaxKind, } from "ts-morph"; -import { getFlubConfig } from "../../config.js"; +/** + * Used by `TagAssertsCommand`. + */ +export interface AssertTaggingPackageConfig { + /** + * Property key is the name of the assert function. + * Property value is the index of the augment to tag. + * @remarks + * The function names are not handled in a scoped/qualified way, so any function imported or declared with that name will have tagging applied. + * This applies to the package it is in. + * @privateRemarks + * See also {@link AssertTaggingConfig.assertionFunctions}. + */ + assertionFunctions: { [functionName: string]: number }; +} /** * Key is the name of the assert function. * Value is the index of the augment to tag. * @remarks * The function names are not handled in a scoped/qualified way, so any function imported or declared with that name will have tagging applied. - * See also {@link AssertTaggingConfig.assertionFunctions}. + * See also {@link AssertTaggingPackageConfig.assertionFunctions}. */ type AssertionFunctions = ReadonlyMap; @@ -161,16 +177,29 @@ export class TagAssertsCommand extends PackageCommand }; const dataMap = new Map(); + const config = cosmiconfig("assertTagging"); for (const pkg of packages) { // Package configuration: // eslint-disable-next-line no-await-in-loop const tsconfigPath = await this.getTsConfigPath(pkg); - const packageConfig = getFlubConfig(pkg.directory).assertTagging; - const assertionFunctions: AssertionFunctions = - packageConfig?.assertionFunctions === undefined - ? defaultAssertionFunctions - : new Map(Object.entries(packageConfig.assertionFunctions)); + // eslint-disable-next-line no-await-in-loop + const packageConfig = await config.search(pkg.directory); + let assertionFunctions: AssertionFunctions; + if (packageConfig === null) { + assertionFunctions = defaultAssertionFunctions; + } else { + const innerConfig = packageConfig.config as AssertTaggingPackageConfig; + // Do some really minimal validation of the configuration. + // TODO: replace this with robust validation, like a strongly typed utility wrapping cosmiconfig and typebox. + assert( + typeof innerConfig.assertionFunctions === "object", + `Assert tagging config in ${packageConfig.filepath} is not valid.`, + ); + assertionFunctions = new Map( + Object.entries(innerConfig.assertionFunctions), + ); + } // load the project based on the tsconfig const project = new Project({ diff --git a/build-tools/packages/build-cli/src/config.ts b/build-tools/packages/build-cli/src/config.ts index 2b6a607b2e05..10dfdc2cfe23 100644 --- a/build-tools/packages/build-cli/src/config.ts +++ b/build-tools/packages/build-cli/src/config.ts @@ -221,22 +221,12 @@ export interface PolicyConfig { * Used by `TagAssertsCommand`. */ export interface AssertTaggingConfig { - /** - * Property key is the name of the assert function. - * Property value is the index of the augment to tag. - * @remarks - * The function names are not handled in a scoped/qualified way, so any function imported or declared with that name will have tagging applied. - * See also {@link AssertTaggingConfig.assertionFunctions} - * - * This applies to the package it is in. - */ - assertionFunctions?: { [functionName: string]: number }; - /** * An array of paths under which assert tagging applies to. If this setting is provided, only packages whose paths * match the regular expressions in this setting will be assert-tagged. * * This is used from the root where flub is run. + * TODO: this should be replaced by package selection flags passed to the command. */ enabledPaths?: RegExp[]; } diff --git a/build-tools/packages/build-cli/src/index.ts b/build-tools/packages/build-cli/src/index.ts index c9e5ea26b964..fd71f09ea0d7 100644 --- a/build-tools/packages/build-cli/src/index.ts +++ b/build-tools/packages/build-cli/src/index.ts @@ -19,3 +19,6 @@ export type { ScriptRequirement, } from "./config.js"; export type { knownReleaseGroups, ReleaseGroup, ReleasePackage } from "./releaseGroups.js"; + +// Exported for use in config files. +export type { AssertTaggingPackageConfig } from "./commands/generate/assertTags.js"; diff --git a/fluidBuild.config.cjs b/fluidBuild.config.cjs index 4208e9b439fd..f24855a3d9e2 100644 --- a/fluidBuild.config.cjs +++ b/fluidBuild.config.cjs @@ -585,9 +585,6 @@ module.exports = { assertTagging: { enabledPaths: [/^common\/lib\/common-utils/i, /^experimental/i, /^packages/i], - assertionFunctions: { - assert: 1, - }, }, // `flub bump` config. These settings influence `flub bump` behavior for a release group. These settings can be diff --git a/packages/dds/test-dds-utils/flub.config.cjs b/packages/dds/test-dds-utils/assertTagging.config.mjs similarity index 51% rename from packages/dds/test-dds-utils/flub.config.cjs rename to packages/dds/test-dds-utils/assertTagging.config.mjs index 6808d55b90a5..b4207e1d036a 100644 --- a/packages/dds/test-dds-utils/flub.config.cjs +++ b/packages/dds/test-dds-utils/assertTagging.config.mjs @@ -3,10 +3,7 @@ * Licensed under the MIT License. */ -"use strict"; - -module.exports = { - assertTagging: { - assertionFunctions: {}, - }, -}; +/** + * @type {import("@fluid-tools/build-cli").AssertTaggingConfig} + */ +export default { assertionFunctions: {} }; diff --git a/packages/dds/tree/assertTagging.config.mjs b/packages/dds/tree/assertTagging.config.mjs new file mode 100644 index 000000000000..8f9138b87013 --- /dev/null +++ b/packages/dds/tree/assertTagging.config.mjs @@ -0,0 +1,14 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +/** + * @type {import("@fluid-tools/build-cli").AssertTaggingConfig} + */ +export default { + assertionFunctions: { + "assert": 1, + "fail": 0, + }, +}; diff --git a/packages/runtime/test-runtime-utils/assertTagging.config.mjs b/packages/runtime/test-runtime-utils/assertTagging.config.mjs new file mode 100644 index 000000000000..cb76723c7bea --- /dev/null +++ b/packages/runtime/test-runtime-utils/assertTagging.config.mjs @@ -0,0 +1,9 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +/** + * @type {import("@fluid-tools/build-cli").AssertTaggingConfig} + */ +export default { assertionFunctions: { assertionFunctions: {} } }; diff --git a/packages/runtime/test-runtime-utils/flub.config.cjs b/packages/test/assertTagging.config.mjs similarity index 51% rename from packages/runtime/test-runtime-utils/flub.config.cjs rename to packages/test/assertTagging.config.mjs index 6808d55b90a5..b4207e1d036a 100644 --- a/packages/runtime/test-runtime-utils/flub.config.cjs +++ b/packages/test/assertTagging.config.mjs @@ -3,10 +3,7 @@ * Licensed under the MIT License. */ -"use strict"; - -module.exports = { - assertTagging: { - assertionFunctions: {}, - }, -}; +/** + * @type {import("@fluid-tools/build-cli").AssertTaggingConfig} + */ +export default { assertionFunctions: {} }; diff --git a/packages/test/flub.config.cjs b/packages/test/flub.config.cjs deleted file mode 100644 index 6808d55b90a5..000000000000 --- a/packages/test/flub.config.cjs +++ /dev/null @@ -1,12 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -"use strict"; - -module.exports = { - assertTagging: { - assertionFunctions: {}, - }, -}; From 46cfaa3db820c0d75b4ce50cfab7ae0dbcf770c7 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Wed, 8 Jan 2025 16:23:59 -0800 Subject: [PATCH 11/15] remove old config --- packages/dds/tree/flub.config.cjs | 15 --------------- 1 file changed, 15 deletions(-) delete mode 100644 packages/dds/tree/flub.config.cjs diff --git a/packages/dds/tree/flub.config.cjs b/packages/dds/tree/flub.config.cjs deleted file mode 100644 index a1b394114022..000000000000 --- a/packages/dds/tree/flub.config.cjs +++ /dev/null @@ -1,15 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -"use strict"; - -module.exports = { - assertTagging: { - assertionFunctions: { - "assert": 1, - "fail": 0, - }, - }, -}; From 5a7b3dc46a183ba60345634e0f8f5165ebb836ad Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Mon, 13 Jan 2025 17:46:36 -0800 Subject: [PATCH 12/15] Document config --- .../build-cli/src/commands/generate/assertTags.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts index 819fb99044f7..f64d305ef55a 100644 --- a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts +++ b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts @@ -65,12 +65,17 @@ interface PackageData { readonly assertionFunctions: AssertionFunctions; } +const configName = "assertTagging"; + export class TagAssertsCommand extends PackageCommand { static readonly summary = "Tags asserts by replacing their message with a unique numerical value."; static readonly description = - "Tagged asserts are smaller because the message string is not included, and they're easier to aggregate for telemetry purposes."; + `Tagged asserts are smaller because the message string is not included, and they're easier to aggregate for telemetry purposes. +Which functions and which of their augments get tagging depends on the configuration which is specified in the package being tagged. +Configuration is searched for in the places listed by https://github.com/cosmiconfig/cosmiconfig?tab=readme-ov-file#usage-for-end-users under the {Name} "${configName}". +The format of the configuration is specified by the "AssertTaggingPackageConfig" type.`; static readonly flags = { disableConfig: Flags.boolean({ @@ -177,7 +182,7 @@ export class TagAssertsCommand extends PackageCommand }; const dataMap = new Map(); - const config = cosmiconfig("assertTagging"); + const config = cosmiconfig(configName); for (const pkg of packages) { // Package configuration: From b2fca3dd03d33b11ebf350f8e9bc979dc85b77eb Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Mon, 13 Jan 2025 17:58:19 -0800 Subject: [PATCH 13/15] Document configs --- packages/dds/test-dds-utils/assertTagging.config.mjs | 5 ++++- packages/test/assertTagging.config.mjs | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/dds/test-dds-utils/assertTagging.config.mjs b/packages/dds/test-dds-utils/assertTagging.config.mjs index b4207e1d036a..5e17f0509cfe 100644 --- a/packages/dds/test-dds-utils/assertTagging.config.mjs +++ b/packages/dds/test-dds-utils/assertTagging.config.mjs @@ -6,4 +6,7 @@ /** * @type {import("@fluid-tools/build-cli").AssertTaggingConfig} */ -export default { assertionFunctions: {} }; +export default { + // Disables assert tagging by listing an empty set of functions that should be tagged. + assertionFunctions: {}, +}; diff --git a/packages/test/assertTagging.config.mjs b/packages/test/assertTagging.config.mjs index b4207e1d036a..5e17f0509cfe 100644 --- a/packages/test/assertTagging.config.mjs +++ b/packages/test/assertTagging.config.mjs @@ -6,4 +6,7 @@ /** * @type {import("@fluid-tools/build-cli").AssertTaggingConfig} */ -export default { assertionFunctions: {} }; +export default { + // Disables assert tagging by listing an empty set of functions that should be tagged. + assertionFunctions: {}, +}; From 9593d26c8062ebb64b8a0cba0cd4082ccd6cd053 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Mon, 13 Jan 2025 19:02:53 -0800 Subject: [PATCH 14/15] Update generated docs --- build-tools/packages/build-cli/docs/generate.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/build-tools/packages/build-cli/docs/generate.md b/build-tools/packages/build-cli/docs/generate.md index 6119b87488f3..a2087ba056f7 100644 --- a/build-tools/packages/build-cli/docs/generate.md +++ b/build-tools/packages/build-cli/docs/generate.md @@ -69,6 +69,11 @@ DESCRIPTION Tagged asserts are smaller because the message string is not included, and they're easier to aggregate for telemetry purposes. + Which functions and which of their augments get tagging depends on the configuration which is specified in the package + being tagged. + Configuration is searched for in the places listed by + https://github.com/cosmiconfig/cosmiconfig?tab=readme-ov-file#usage-for-end-users under the {Name} "assertTagging". + The format of the configuration is specified by the "AssertTaggingPackageConfig" type. ``` _See code: [src/commands/generate/assertTags.ts](https://github.com/microsoft/FluidFramework/blob/main/build-tools/packages/build-cli/src/commands/generate/assertTags.ts)_ From 240b8ab2ae87ade34262457f14422ecb8e8a21ac Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Thu, 16 Jan 2025 11:16:25 -0800 Subject: [PATCH 15/15] Restrict config to only config.mjs --- build-tools/packages/build-cli/docs/generate.md | 4 ++-- .../packages/build-cli/src/commands/generate/assertTags.ts | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/build-tools/packages/build-cli/docs/generate.md b/build-tools/packages/build-cli/docs/generate.md index a2087ba056f7..71dd4b054421 100644 --- a/build-tools/packages/build-cli/docs/generate.md +++ b/build-tools/packages/build-cli/docs/generate.md @@ -71,8 +71,8 @@ DESCRIPTION purposes. Which functions and which of their augments get tagging depends on the configuration which is specified in the package being tagged. - Configuration is searched for in the places listed by - https://github.com/cosmiconfig/cosmiconfig?tab=readme-ov-file#usage-for-end-users under the {Name} "assertTagging". + Configuration is searched by walking from each package's directory up to its parents recursively looking for the first + file matching one of ["assertTagging.config.mjs"]. The format of the configuration is specified by the "AssertTaggingPackageConfig" type. ``` diff --git a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts index f64d305ef55a..2faf8a1e6885 100644 --- a/build-tools/packages/build-cli/src/commands/generate/assertTags.ts +++ b/build-tools/packages/build-cli/src/commands/generate/assertTags.ts @@ -66,6 +66,7 @@ interface PackageData { } const configName = "assertTagging"; +const searchPlaces = [`${configName}.config.mjs`]; export class TagAssertsCommand extends PackageCommand { static readonly summary = @@ -74,7 +75,7 @@ export class TagAssertsCommand extends PackageCommand static readonly description = `Tagged asserts are smaller because the message string is not included, and they're easier to aggregate for telemetry purposes. Which functions and which of their augments get tagging depends on the configuration which is specified in the package being tagged. -Configuration is searched for in the places listed by https://github.com/cosmiconfig/cosmiconfig?tab=readme-ov-file#usage-for-end-users under the {Name} "${configName}". +Configuration is searched by walking from each package's directory up to its parents recursively looking for the first file matching one of ${JSON.stringify(searchPlaces)}. The format of the configuration is specified by the "AssertTaggingPackageConfig" type.`; static readonly flags = { @@ -182,7 +183,7 @@ The format of the configuration is specified by the "AssertTaggingPackageConfig" }; const dataMap = new Map(); - const config = cosmiconfig(configName); + const config = cosmiconfig(configName, { searchPlaces }); for (const pkg of packages) { // Package configuration: