Enable support for per package assert tagging configuration #23452

CraigMacomber · 2025-01-03T23:43:20Z

Description

Refactor assert tagger so its relies much less on function side effects and global state.

Make assert functions configuration per package instead of global.

Remove exclusion of test packages: exclude them as needed using config.

Configure tree to tag its "fail" function. This will enable a small bundle size improvement for tree.

Reviewer Guidance

The review process is outlined on this wiki page.

Copilot reviewed 5 out of 12 changed files in this pull request and generated no comments.

Files not reviewed (7)

packages/dds/test-dds-utils/flub.config.cjs: Evaluated as low risk
packages/runtime/test-runtime-utils/flub.config.cjs: Evaluated as low risk
build-tools/packages/build-cli/src/commands/generate/assertTags.ts: Evaluated as low risk
build-tools/packages/build-cli/src/config.ts: Evaluated as low risk
packages/dds/tree/flub.config.cjs: Evaluated as low risk
packages/dds/tree/src/feature-libraries/sequence-field/sequenceFieldCodecV1.ts: Evaluated as low risk
packages/dds/tree/src/simple-tree/toMapTree.ts: Evaluated as low risk

Comments suppressed due to low confidence (1)

packages/dds/tree/src/simple-tree/api/treeNodeApi.ts:177

The error message should include the specific field information to aid in debugging. Suggest reverting to the original message: fail(Could not find stored key '${field}' in schema.).

fail("Could not find stored key in schema.")

CraigMacomber · 2025-01-07T18:36:31Z

build-tools/packages/build-cli/src/commands/generate/assertTags.ts


-	private collectAssertData(tsconfigPath: string): void {
-		// TODO: this can probably be removed now


Removing this caused many more packages to be processed, like test-dds-utils, test-runtime-utils and everything in packages/test . See the config files added to filter them out.

build-tools/packages/build-cli/src/commands/generate/assertTags.ts

tylerbutler · 2025-01-07T19:24:40Z

build-tools/packages/build-cli/src/config.ts

+	 * 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.


I think you could make it work that way for this command. fluid-build supports task definitions in both places, but flub has not historically needed such a capability outside of the typetest commands. I'm inclined to leave handling individual package config to individual commands for now.

@zhenmichael Something to keep an eye on as you port commands to the new infrastructure. Are there more commands that could benefit from a general-purpose per-package config loading capability? Maybe that feature could be baked into the new PackageCommand base class from the beginning?

If you are going to look at general things we want for configuration, I think the big one thats missing is any sort of schema/validation system.

Using a library like typebox to give you errors if you have a typo in your config field names or invalid structure would be nice (instead of just casting config data parsed as json to a type an hoping its right). Would have saved me some annoyance when I messed up the nesting working on this PR.

Using typescript types in the JS config files helps prevent a lot of these errors. You're absolutely right about the stuff in package.json.

tylerbutler · 2025-01-07T19:36:53Z

packages/dds/test-dds-utils/flub.config.cjs

I think this will mean that the root flub config is never loaded. The config nearest to the working dir is loaded, and there's no implied inheritance or merging. You could import the root config here, modify it and then export it.

But I am conflicted about this approach. In the type tests, the one other place where this sort of scenario (command/package-specific config) exists, the per-package config is in the package.json for the package and the type test commands load the settings there. I like this because it makes it clearer that this config is not flub config, really, but config for a specific command.

That said, I don't think we should pollute the top-level package.json 'namespace,' so maybe a flubCommandConfig node that individual commands can add entries to?

build-tools/packages/build-cli/src/commands/generate/assertTags.ts

tylerbutler · 2025-01-07T19:47:20Z

build-tools/packages/build-cli/src/commands/generate/assertTags.ts

+	// 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.


Great observation. This is a side effect of the evolution of the code and the developers writing it. :)

@zhenmichael Let's chat about the specific ideas Craig shared and integrate them into the design for PackageCommand vnext. The most critical thing to decide in order to unblock this PR is where the per-package config should live (see other comments). Any enhancements to the selection/filtering should live in build-infrastructure with the new functions there. Some of the things called out here will likely be addressed as a side effect of building on the new selection/filtering stuff.

If PackageCommand allowed passing both --releaseGroup and --package at the same time then we could use that to replace the regex filter. Thats likely the simplest solution since we could just pass in --releaseGroup client --package @fluidframework/common-utils instead of --all.

If we do want to keep the regex functionality, it can be kept and moved to PackageCommand along with its other filtering options.

Supporting multiple selection flags seems trivial, so I'm going to try and make that change.

This should do it: #23511

tylerbutler · 2025-01-07T20:26:00Z

build-tools/packages/build-cli/src/commands/generate/assertTags.ts

+			const tsconfigPath = await this.getTsConfigPath(pkg);
+			const packageConfig = getFlubConfig(pkg.directory).assertTagging;


Ahh, I see you're loading the package config here. I think putting it in package.json will be clearer and lets you get rid of some of this code since you can just get the config from the package directly.

Are you suggesting to:

not use FlubConfig for this, or

to put it in the package.json as part of the FlubConfig?

I think you mean 1, which makes sense, though I suspect it won't be less code just less over all coupling.

I'm suggesting putting the settings in package.json, but not with the existing FlubConfig interface. Maybe it makes sense to use the AssertTaggingConfig interface? I was imagining something like this in the individual packages:

{ "flubCommandConfig": "generate:asserts": { "assertionFunctions": { "assert": 1, "fail": 0, } }

We could get rid of the "flubCommandConfig" layer, and maybe that's a bad name anyway. I was trying to avoid adding a bunch of stuff at the top level, but I'm more pragmatic than principled in this case.

Keeping the ability to have one config file cover all 13 packages in packages/test is useful, so I kept the use of cosmic config, but make it use its own config. THis should prevent this folder config from causing issues with any other configurations.

build-tools/packages/build-cli/src/commands/generate/assertTags.ts

alexvy86 · 2025-01-07T20:53:51Z

packages/dds/tree/src/util/utils.ts

+export function fail(message: string | number): never {
+	throw new Error(
+		typeof message === "number" ? `0x${message.toString(16).padStart(3, "0")}` : message,
+	);


At this point would it make sense to define it in terms of assert(false, ...)?

doing that is an error, since assert tagging errors if you pass something other than a constant string to assert. Its possible to work around that, but thats why I didn't do exactly as you suggest.

Co-authored-by: Tyler Butler <[email protected]>

CraigMacomber · 2025-01-07T23:13:10Z

We seem to have a second implementation of assert tagging in build-tools/packages/build-cli/src/library/repoPolicyCheck/assertShortCode.ts . The implications of doing changes to one copy of the code and not the other are not super clear, but will likely cause issues. I think this should be deduplicated before this changes goes forward. Hopefully said deduplication can be done without conflicting with this change too much.

(Edit: it has now been removed)

…to failTag

CraigMacomber added 4 commits January 3, 2025 14:52

Setup tagging for tree's "fail"

aadc2e0

Fix fail tagging

c16f987

remove fail templates

5e1ce1f

Update fail to allow tagged messages

89ce236

Merge branch 'main' into failTag

3d5fee3

CraigMacomber marked this pull request as ready for review January 7, 2025 17:49

Copilot bot review requested due to automatic review settings January 7, 2025 17:49

CraigMacomber requested a review from a team as a code owner January 7, 2025 17:49

Copilot AI reviewed Jan 7, 2025

View reviewed changes

Work on comments

5c06f20

CraigMacomber commented Jan 7, 2025

View reviewed changes

fix error aggregation and extend comment

e55dd41

tylerbutler reviewed Jan 7, 2025

View reviewed changes

alexvy86 reviewed Jan 7, 2025

View reviewed changes

Apply suggestions from code review

a67474c

Co-authored-by: Tyler Butler <[email protected]>

CraigMacomber added 7 commits January 7, 2025 15:19

workaround assert tagging to make fail call assert

2c8fe5c

Fix newAssetFiles typo

137a6c2

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

472cf03

…to failTag

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

fbac2c2

…to failTag

Give assert tagging its own config

18ae12c

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

c400e3c

…to failTag

remove old config

46cfaa3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable support for per package assert tagging configuration #23452

Enable support for per package assert tagging configuration #23452

CraigMacomber commented Jan 3, 2025 •

edited

Loading

CraigMacomber Jan 7, 2025

tylerbutler Jan 7, 2025

CraigMacomber Jan 8, 2025

tylerbutler Jan 8, 2025

tylerbutler Jan 7, 2025

tylerbutler Jan 7, 2025

CraigMacomber Jan 8, 2025

CraigMacomber Jan 8, 2025

CraigMacomber Jan 8, 2025

tylerbutler Jan 7, 2025

CraigMacomber Jan 8, 2025

tylerbutler Jan 8, 2025

CraigMacomber Jan 9, 2025

alexvy86 Jan 7, 2025

CraigMacomber Jan 7, 2025

CraigMacomber commented Jan 7, 2025 •

edited

Loading


		private collectAssertData(tsconfigPath: string): void {
		// TODO: this can probably be removed now

		const tsconfigPath = await this.getTsConfigPath(pkg);
		const packageConfig = getFlubConfig(pkg.directory).assertTagging;

Enable support for per package assert tagging configuration #23452

Are you sure you want to change the base?

Enable support for per package assert tagging configuration #23452

Conversation

CraigMacomber commented Jan 3, 2025 • edited Loading

Description

Reviewer Guidance

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CraigMacomber commented Jan 7, 2025 • edited Loading

CraigMacomber commented Jan 3, 2025 •

edited

Loading

CraigMacomber commented Jan 7, 2025 •

edited

Loading