fix(client-PropertyDDS): correct MSG constants by jason-ha · Pull Request #26971 · microsoft/FluidFramework

jason-ha · 2026-04-08T05:10:32Z

PR #7322 "feat: optional property in PropertyDDS"
introduced use of several MSG constants that were never defined.

OVERRIDEN_PROP_MUST_HAVE_SAME_CONTEXT_AS_BASE_TYPE use was replaced by OVERRIDEN_PROP_MUST_HAVE_SAME_FIELD_VALUES_AS_BASE_TYPE (but never defined).
CANNOT_INSERT_UNKNOWN_PROPERTY, MISMATCHING_PROPERTY_TYPEID, and CANNOT_REMOVE_NON_OPTIONAL_PROP uses were added without defining them.

Tests improperly validated that those were thrown, but since they were undefined in test code too, the tests didn't fail.

checkJs was enabled for property-properties test build to find those issues and several more.

Some additional production typing changes were made where no runtime changes were required.
global.d.ts updates with the additional globals configured by setup.cjs (@types/chai devDep added)
Some incorrect test code was updated for proper execution and build validation.
- Removed unused variables / imports.
- Corrected duplicate declarations.
- Fixed BaseProperty.MODIFIED_STATE_FLAGS.DIRTY references.
- Fixed ValueMapProperty add/remove test expects.
- Workaround console.warn typing to access spy .callCount property.
- Use intermediate SerializedChangeSet variable in StringProperty Rebasing tests.

PR microsoft#7322 "feat: optional property in PropertyDDS" introduced use of several MSG constants that were never defined. - OVERRIDEN_PROP_MUST_HAVE_SAME_CONTEXT_AS_BASE_TYPE use was replaced by OVERRIDEN_PROP_MUST_HAVE_SAME_FIELD_VALUES_AS_BASE_TYPE (but never defined). - CANNOT_INSERT_UNKNOWN_PROPERTY, MISMATCHING_PROPERTY_TYPEID, and CANNOT_REMOVE_NON_OPTIONAL_PROP uses were added without defining them. Tests improperly validated that those were thrown, but since they were `undefined` in test code too, the tests didn't fail. `checkJs` was enabled for property-properties test build to find those issues and several more. - Some additional production typing changes were made where no runtime changes were required. - global.d.ts updates with the additional globals configured by setup.cjs (`@types/chai` devDep added) - Some incorrect test code was updated for proper execution and build validation. - Removed unused variables / imports. - Corrected duplicate declarations. - Fixed BaseProperty.MODIFIED_STATE_FLAGS.DIRTY references. - Fixed ValueMapProperty add/remove test `expect`s. - Workaround `console.warn` typing to access spy `.callCount` property. - Use intermediate SerializedChangeSet variable in StringProperty Rebasing tests.

Copilot

Pull request overview

This PR fixes missing/incorrect MSG constants introduced in PropertyDDS changes, and tightens JS test type-checking by enabling checkJs for the property-properties test build.

Changes:

Add the previously-referenced-but-undefined MSG constants (and update one renamed constant).
Enable checkJs for property-properties tests and add required global typings / dev deps.
Clean up multiple tests to satisfy type-checking and correct assertions (e.g., ValueMapProperty expectations, duplicate var declarations).

Reviewed changes

Copilot reviewed 16 out of 18 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
pnpm-lock.yaml	Locks newly-added `@types/chai` dependency.
experimental/PropertyDDS/packages/property-properties/package.json	Adds `@types/chai` dev dependency for test typing.
experimental/PropertyDDS/packages/property-properties/src/test/tsconfig.json	Enables `checkJs` and adds `chai`/`sinon` to `types`.
experimental/PropertyDDS/packages/property-properties/src/test/global.d.ts	Declares globals provided by `test/setup.cjs` for JS type-checking.
experimental/PropertyDDS/packages/property-common/src/constants.ts	Defines missing `MSG` entries and renames one `PropertyFactoryError` key.
experimental/PropertyDDS/packages/property-changeset/src/utils.ts	Tightens typings for a couple of utilities used by tests/consumers.
experimental/PropertyDDS/packages/property-changeset/src/changeset.ts	Makes `_stripReversibleChangeSet` parameter optional.
experimental/PropertyDDS/packages/property-properties/src/test/*/.spec.js	Updates various tests to compile/run correctly under `checkJs` and to validate correct `MSG` constants.

Files not reviewed (1)

pnpm-lock.yaml: Language not supported

Copilot · 2026-04-08T05:16:54Z

experimental/PropertyDDS/packages/property-changeset/src/utils.ts

 	export function getChangesByType(
 		in_typeid: string,
 		in_changeSet: SerializedChangeSet,
-		in_excludeTypeids: boolean,
+		in_excludeTypeids?: boolean,
 	): { insert?: object; modify?: object } {


getChangesByType can throw at runtime when in_excludeTypeids is truthy but no matching changes were found for insert and/or modify (since result.insert / result.modify may be undefined and Object.keys(undefined) will crash). Initialize result.insert/result.modify to {} up front, or guard with nullish checks before iterating/stripping typeids.

Copilot · 2026-04-08T05:16:54Z

experimental/PropertyDDS/packages/property-common/src/constants.ts

 	 *
-	 * Overridden properties must have same context as the base type.
+	 * Overridden properties must have same field values as the base type.
 	 */
-	OVERRIDEN_PROP_MUST_HAVE_SAME_CONTEXT_AS_BASE_TYPE:
-		"PF-067: Overridden properties must have same context as the base type: ",
+	OVERRIDEN_PROP_MUST_HAVE_SAME_FIELD_VALUES_AS_BASE_TYPE:
+		"PF-067: Overridden properties must have same field values as the base type: ",


Renaming OVERRIDEN_PROP_MUST_HAVE_SAME_CONTEXT_AS_BASE_TYPE to OVERRIDEN_PROP_MUST_HAVE_SAME_FIELD_VALUES_AS_BASE_TYPE removes an exported MSG key and can break any downstream callers (even if internal usage has moved). Consider keeping the old key as a deprecated alias to the new one (same message value) to preserve backward compatibility.

Copilot · 2026-04-08T05:16:54Z

experimental/PropertyDDS/packages/property-properties/src/test/properties/baseProperty.spec.js

 		 * creates a workspace
-		 * @return {property-properties.Workspace} workspace
+		 * @return {Promise<import("../../index.js").NodeProperty>} workspace
+		 * @privateRemarks
+		 * The return type can be `Workspace` once package is updated to support modules. Then
+		 * {Promise<import("@fluid-experimental/property-properties").Workspace>} may be used.


The JSDoc return type import("../../index.js") does not resolve from src/test/properties (there is no src/test/index.js), so checkJs is likely to report a missing module. Use a resolvable specifier (e.g. import("../..") or import("../../index")) and consider updating the description/variable name since this helper returns a NodeProperty, not a Workspace.

Copilot · 2026-04-08T05:16:55Z

experimental/PropertyDDS/packages/property-properties/src/test/utils.spec.js

+			let res = Utils.excludePathsFromChangeSet(
+				changeset,
+				// @ts-expect-error - testing the case when in_paths argument is a string instead of an array
+				"assets[Prop3]",
+			);


These @ts-expect-error suppressions suggest the runtime accepts in_paths as a single string, but the TypeScript signature only allows string[]. Instead of suppressing in tests, consider widening Utils.excludePathsFromChangeSet typing to accept string | string[] (and possibly undefined), matching its runtime behavior.

Copilot · 2026-04-08T05:16:55Z

experimental/PropertyDDS/packages/property-properties/src/test/utils.spec.js

+			expect(
+				// @ts-expect-error - testing the case when in_paths argument is not passed
+				Utils.excludePathsFromChangeSet(changeset),
+			).to.be.deep.equal(changeset);
+			expect(
+				Utils.excludePathsFromChangeSet(
+					changeset,
+					// @ts-expect-error - testing the case when in_paths argument is an empty string
+					"",
+				),
+			).to.be.deep.equal(changeset);


These @ts-expect-error cases cover omitted/empty-string in_paths, which excludePathsFromChangeSet currently treats as a no-op at runtime. If that behavior is intended, it would be better to reflect it in the function’s TypeScript signature (e.g. in_paths?: string | string[]) so tests don't need error suppressions.

Suggested change

expect(

// @ts-expect-error - testing the case when in_paths argument is not passed

Utils.excludePathsFromChangeSet(changeset),

).to.be.deep.equal(changeset);

expect(

Utils.excludePathsFromChangeSet(

changeset,

// @ts-expect-error - testing the case when in_paths argument is an empty string

"",

),

).to.be.deep.equal(changeset);

/** @type {any} */

const excludePathsFromChangeSet = Utils.excludePathsFromChangeSet;

/** @type {any} */

const emptyPaths = "";

expect(excludePathsFromChangeSet(changeset)).to.be.deep.equal(changeset);

expect(Utils.excludePathsFromChangeSet(changeset, emptyPaths)).to.be.deep.equal(

changeset,

);

Copilot · 2026-04-08T05:16:55Z

experimental/PropertyDDS/packages/property-changeset/src/changeset.ts

 	 * in a later release.
 	 * @param in_withoutRoot - Bypass a fix where the root of a changeset is cleaned
 	 */
-	public _stripReversibleChangeSet(in_withoutRoot: boolean) {
+	public _stripReversibleChangeSet(in_withoutRoot?: boolean) {
 		const callback = function (in_context) {


_stripReversibleChangeSet now takes an optional parameter, but the JSDoc still documents in_withoutRoot as required. Update the @param documentation (and/or provide a default value) so callers understand the default behavior when the argument is omitted.

Copilot AI review requested due to automatic review settings April 8, 2026 05:10

Copilot started reviewing on behalf of jason-ha April 8, 2026 05:11 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

test(client-PropertyDDS): prefixes for type-safe static import use

1acd37e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(client-PropertyDDS): correct MSG constants#26971

fix(client-PropertyDDS): correct MSG constants#26971
jason-ha wants to merge 2 commits intomicrosoft:mainfrom
jason-ha:propertyDDS/fix-MSG-constants-and-test-build

jason-ha commented Apr 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jason-ha commented Apr 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants