fix: tree: make field assignment safer #23053

CraigMacomber · 2024-11-11T22:33:00Z

Description

Fixes two typing issues related to assignment of fields.

One issue is tracked by AB:9129

See changeset for details.

Breaking Changes

It's possible that an application relied on this unsafety to assign to fields with non-exact schema. This is unlikely since creating such nodes is not allowed by the types already.

Assignment of identifier fields unconditionally errors at runtime, so it is unlikely anyone depended on that compiling.

Reviewer Guidance

The review process is outlined on this wiki page.

CraigMacomber · 2024-11-11T22:33:46Z

packages/dds/tree/src/test/simple-tree/objectNode.spec.ts

@@ -234,10 +236,99 @@ describeHydration(
 				}) {}
 				const n = init(HasId, {});
 				assert.throws(() => {
-					// TODO: AB:9129: this should not compile
+					// @ts-expect-error this should not compile


This is the test which was tracking the known part of this issue. It now works as expected for non-recursive objects.

CraigMacomber · 2024-11-11T22:34:32Z

packages/dds/tree/src/test/shared-tree-core/sharedTreeCore.spec.ts

@@ -294,9 +294,9 @@ describe("SharedTreeCore", () => {
 		});

 		const sf = new SchemaFactory("0x4a6 repro");
-		const TestNode = sf.objectRecursive("test node", {
+		class TestNode extends sf.objectRecursive("test node", {


This change is not required: I simply made it to get much better compile errors when debugging, and left it so other in the future can continue to have much cleaner errors if this code breaks again.

packages/dds/tree/src/simple-tree/objectNode.ts

Josmithr · 2024-11-11T23:16:38Z

packages/dds/tree/src/simple-tree/objectNode.ts

+		: never;
+
+/**
+ * Suitable for assignment.


Anything more we can say here?

msfluid-bot

Code Coverage Summary

↓ packages.dds.tree.src.simple-tree.api:
Line Coverage Change: -0.05% Branch Coverage Change: No change

Metric Name	Baseline coverage	PR coverage	Coverage Diff
Branch Coverage	88.76%	88.76%	→ No change
Line Coverage	82.28%	82.23%	↓ -0.05%

↑ packages.dds.tree.src.simple-tree:
Line Coverage Change: 0.03% Branch Coverage Change: No change

Metric Name	Baseline coverage	PR coverage	Coverage Diff
Branch Coverage	94.07%	94.07%	→ No change
Line Coverage	97.23%	97.26%	↑ 0.03%

Baseline commit: cc3fb6d
Baseline build: 306250
Happy Coding!!

Code coverage comparison check passed!!

msfluid-bot · 2024-11-11T23:20:26Z

⯅ @fluid-example/bundle-size-tests: +245 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	464.33 KB	464.36 KB	⯅ +35 Bytes
azureClient.js	563.18 KB	563.23 KB	⯅ +49 Bytes
connectionState.js	724 Bytes	724 Bytes	■ No change
containerRuntime.js	262.48 KB	262.5 KB	⯅ +14 Bytes
fluidFramework.js	426.84 KB	426.85 KB	⯅ +14 Bytes
loader.js	134.18 KB	134.19 KB	⯅ +14 Bytes
map.js	42.71 KB	42.71 KB	⯅ +7 Bytes
matrix.js	148.36 KB	148.36 KB	⯅ +7 Bytes
odspClient.js	528.97 KB	529.02 KB	⯅ +49 Bytes
odspDriver.js	97.88 KB	97.9 KB	⯅ +21 Bytes
odspPrefetchSnapshot.js	42.81 KB	42.83 KB	⯅ +14 Bytes
sharedString.js	164.26 KB	164.27 KB	⯅ +7 Bytes
sharedTree.js	417.3 KB	417.3 KB	⯅ +7 Bytes
Total Size	3.37 MB	3.37 MB	⯅ +245 Bytes

Baseline commit: cc3fb6d

Generated by 🚫 dangerJS against 90a79f5

Co-authored-by: Joshua Smithrud <[email protected]>

CraigMacomber · 2024-11-12T00:43:47Z

I missed a case in the implementation. It does not handler optional fields very well.

Also looking at possible usage, I think this change should be paired with a new API, maybe TreeAlpha.setField(node, property, content) which can be use in cases where this restriction is overly aggressive due to microsoft/TypeScript#43826

That would provide an option with typing that matches ArrayNode.insert() and TreeView.root.

Copilot reviewed 5 out of 17 changed files in this pull request and generated no suggestions.

Files not reviewed (12)

packages/dds/tree/api-report/tree.legacy.public.api.md: Evaluated as low risk
packages/dds/tree/api-report/tree.alpha.api.md: Evaluated as low risk
.changeset/metal-sloths-join.md: Evaluated as low risk
packages/dds/tree/src/index.ts: Evaluated as low risk
packages/dds/tree/src/simple-tree/index.ts: Evaluated as low risk
packages/dds/tree/src/test/shared-tree-core/sharedTreeCore.spec.ts: Evaluated as low risk
packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md: Evaluated as low risk
packages/framework/fluid-framework/api-report/fluid-framework.public.api.md: Evaluated as low risk
packages/framework/fluid-framework/api-report/fluid-framework.legacy.public.api.md: Evaluated as low risk
packages/dds/tree/api-report/tree.legacy.alpha.api.md: Evaluated as low risk
packages/dds/tree/src/simple-tree/api/typesUnsafe.ts: Evaluated as low risk
packages/framework/fluid-framework/api-report/fluid-framework.legacy.alpha.api.md: Evaluated as low risk

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

…to fieldSafe

github-actions · 2024-11-12T22:57:46Z

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:linkcheck /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test ci:start 1313 linkcheck:full

1: starting server using command "npm run ci:start"
and when url "[ 'http://127.0.0.1:1313' ]" is responding with HTTP status code 200
running tests using command "npm run linkcheck:full"


> [email protected] ci:start
> http-server ./public --port 1313 --silent


> [email protected] linkcheck:full
> npm run linkcheck:fast -- --external


> [email protected] linkcheck:fast
> linkcheck http://localhost:1313 --skip-file skipped-urls.txt --external

Crawling...

Stats:
  443722 links
    3414 destination URLs
       2 URLs ignored
       0 warnings
       0 errors

Make fields safer

aea7d62

CraigMacomber requested review from a team as code owners November 11, 2024 22:33

CraigMacomber commented Nov 11, 2024

View reviewed changes

Josmithr reviewed Nov 11, 2024

View reviewed changes

packages/dds/tree/src/simple-tree/objectNode.ts Outdated Show resolved Hide resolved

Josmithr reviewed Nov 11, 2024

View reviewed changes

packages/dds/tree/src/simple-tree/objectNode.ts

: never;

/**

* Suitable for assignment.

Copy link

Contributor

Josmithr Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything more we can say here?

msfluid-bot reviewed Nov 11, 2024

View reviewed changes

Update packages/dds/tree/src/simple-tree/objectNode.ts

ec64de4

Co-authored-by: Joshua Smithrud <[email protected]>

frankmueller-msft requested a review from Copilot November 12, 2024 18:59

Copilot AI reviewed Nov 12, 2024

View reviewed changes

CraigMacomber added 3 commits November 12, 2024 14:42

Fixes for optional

4119174

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

a90e167

…to fieldSafe

FIx build

90a79f5

CraigMacomber marked this pull request as draft November 13, 2024 23:36

CraigMacomber mentioned this pull request Nov 13, 2024

feat: customizeSchemaTyping #23084

Draft

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: tree: make field assignment safer #23053

fix: tree: make field assignment safer #23053

CraigMacomber commented Nov 11, 2024

CraigMacomber Nov 11, 2024

CraigMacomber Nov 11, 2024

Josmithr Nov 11, 2024

msfluid-bot left a comment •

edited

Loading

msfluid-bot commented Nov 11, 2024 •

edited

Loading

CraigMacomber commented Nov 12, 2024

github-actions bot commented Nov 12, 2024

fix: tree: make field assignment safer #23053

Are you sure you want to change the base?

fix: tree: make field assignment safer #23053

Conversation

CraigMacomber commented Nov 11, 2024

Description

Breaking Changes

Reviewer Guidance

CraigMacomber Nov 11, 2024

Choose a reason for hiding this comment

CraigMacomber Nov 11, 2024

Choose a reason for hiding this comment

Josmithr Nov 11, 2024

Choose a reason for hiding this comment

msfluid-bot left a comment • edited Loading

Choose a reason for hiding this comment

Code Coverage Summary

Code coverage comparison check passed!!

msfluid-bot commented Nov 11, 2024 • edited Loading

CraigMacomber commented Nov 12, 2024

Choose a reason for hiding this comment

github-actions bot commented Nov 12, 2024

linkcheck output

msfluid-bot left a comment •

edited

Loading

msfluid-bot commented Nov 11, 2024 •

edited

Loading