tree: handle schema contravariantly when generating input types #22874

CraigMacomber · 2024-10-22T20:48:10Z

Description

Handle schema contravariantly when generating input types.

Breaking Changes

No runtime changes, but typing tweaks could break users on non-exact schema, and the enum schema APIs have been tweaked to avoid relying on non-exact schema, see changeset.

Reviewer Guidance

The review process is outlined on this wiki page.

…to contra-schema

## Description Misc improvements found when working on #22874 . Many of these are places where schema types were not captured properly, or incorrect typing wasn't detected due to the incorrect variance. ## Breaking Changes The change to `InsertableObjectFromSchemaRecord` could impact something: it's strictly a bug fix but its unclear what implications its wrong version could have. I suspect no realistic code would be broken by these more correct types, but it's possible something could be impacted.

…to contra-schema

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

msfluid-bot

Code Coverage Summary

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

Metric Name	Baseline coverage	PR coverage	Coverage Diff
Branch Coverage	86.53%	86.53%	→ No change
Line Coverage	82.12%	81.68%	↓ -0.44%

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

Metric Name	Baseline coverage	PR coverage	Coverage Diff
Branch Coverage	93.39%	93.39%	→ No change
Line Coverage	97.22%	97.23%	↑ 0.01%

↑ packages.dds.tree.src.simple-tree:
Line Coverage Change: 0.09% Branch Coverage Change: 0.01%

Metric Name	Baseline coverage	PR coverage	Coverage Diff
Branch Coverage	93.29%	93.30%	↑ 0.01%
Line Coverage	97.13%	97.22%	↑ 0.09%

↑ packages.dds.tree.src.events:
Line Coverage Change: 2.09% Branch Coverage Change: No change

Metric Name	Baseline coverage	PR coverage	Coverage Diff
Branch Coverage	92.00%	92.00%	→ No change
Line Coverage	68.46%	70.55%	↑ 2.09%

↑ packages.dds.tree.src.util:
Line Coverage Change: 6.23% Branch Coverage Change: 0.40%

Metric Name	Baseline coverage	PR coverage	Coverage Diff
Branch Coverage	96.34%	96.74%	↑ 0.40%
Line Coverage	73.80%	80.03%	↑ 6.23%

Baseline commit: a1b4cdd
Baseline build: 303010
Happy Coding!!

Code coverage comparison check passed!!

msfluid-bot · 2024-10-23T01:33:49Z

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

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	464.08 KB	464.11 KB	⯅ +35 Bytes
azureClient.js	561.65 KB	561.7 KB	⯅ +49 Bytes
connectionState.js	724 Bytes	724 Bytes	■ No change
containerRuntime.js	261.67 KB	261.68 KB	⯅ +14 Bytes
fluidFramework.js	406.28 KB	406.29 KB	⯅ +14 Bytes
loader.js	134.16 KB	134.18 KB	⯅ +14 Bytes
map.js	42.71 KB	42.71 KB	⯅ +7 Bytes
matrix.js	148.54 KB	148.54 KB	⯅ +7 Bytes
odspClient.js	528.18 KB	528.23 KB	⯅ +49 Bytes
odspDriver.js	97.84 KB	97.86 KB	⯅ +21 Bytes
odspPrefetchSnapshot.js	42.81 KB	42.82 KB	⯅ +14 Bytes
sharedString.js	164.73 KB	164.74 KB	⯅ +7 Bytes
sharedTree.js	396.74 KB	396.74 KB	⯅ +7 Bytes
Total Size	3.32 MB	3.32 MB	⯅ +245 Bytes

Baseline commit: a1b4cdd

Generated by 🚫 dangerJS against 23bfe4c

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

tylerbutler

Just some nits. Approved for docs.

.changeset/shy-dots-prove.md

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

.changeset/few-cobras-sing.md

Josmithr · 2024-10-24T17:15:52Z

.changeset/shy-dots-prove.md

+const view = sharedTree.viewWith(config);
+
+// Does not compile since setter for root is typed `never` due to imprecise schema.
+view.root = [];


Curious: what does the error look like in this case? Is it intelligible?

its cannot assign [] to never I believe. I know what that means, but not sure if others would.

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

Josmithr · 2024-10-24T17:51:58Z

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

+ * @see {@link Input}
+ *
+ * @typeparam TSchema - Schema to process.
+ * @typeparam TResult - Do not specify: default value used as implementation detail.


I take it there is no reasonable way for us to hide these parameters that we strictly want to be inferred? I can't think of one, but you know TypeScript's generics way better than I do.

Adding extra dummy parameters is just a way to have variables in these type computations which can be reused. I think its cleaner than making an additional "inner" type which we also would have to export. We could also just copy past the expression into every use site.

I don't find either of those workarounds better, but they are options we could take instead.

In this specific case though these docs are stale and there is no such parameter: oops!

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

Josmithr · 2024-10-24T17:58:26Z

packages/dds/tree/package.json

@@ -229,13 +229,15 @@
 	"typeValidation": {
 		"broken": {
 			"TypeAlias_InsertableTreeFieldFromImplicitField": {


Sanity check: these are all @system or @internal exports?

Type tests are basically meaningless for generic types since they just pass in any, and we don't actually define what using a schema of any is supposed to result in. There are real and important breaking changes that these do not catch (like those called out in the changeset: type tests didn't detect those), and non breaking changes (like adding a defaulted parameter you aren't allowed to use) which it considers breaking (as is the case for this one).

And no, these are not all system. This one is just public for example.

Also, @internal types won't be included here since they are filtered based on release tag. We have some public type exported in the InternalTypes module, but those are not actually @internal just a flawed attempted to try and make systems types not get mixed in with the others.

Josmithr

Left a handful of suggestions and questions. Biggest question: how confident are we that the type-wise breaking changes made here will not disrupt the majority of users?

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

CraigMacomber · 2024-10-24T22:32:22Z

Left a handful of suggestions and questions. Biggest question: how confident are we that the type-wise breaking changes made here will not disrupt the majority of users?

It's hard to tell. It did break our in repo ai-collab code, but it's doing non-schema aware editing. I don't really know how many people are doing things that we never intended to support and are unsafe. I'll do a test build and try it in the examples repo.

alexvy86

Didn't go through the functional changes, just looked at the changesets and left a couple of suggestions, but they otherwise look good. Definitely worth getting more approvals, particularly from API council given the breaking change.

.changeset/shy-dots-prove.md

CraigMacomber · 2024-10-29T18:54:21Z

Left a handful of suggestions and questions. Biggest question: how confident are we that the type-wise breaking changes made here will not disrupt the majority of users?

It's hard to tell. It did break our in repo ai-collab code, but it's doing non-schema aware editing. I don't really know how many people are doing things that we never intended to support and are unsafe. I'll do a test build and try it in the examples repo.

I have done the test integration into the examples repo. It has some preexisting problems, but I don't see anything new from these changes.

github-actions · 2024-10-29T19:17:05Z

🔗 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:
  436034 links
    3364 destination URLs
       2 URLs ignored
       0 warnings
       0 errors

CraigMacomber added 2 commits October 22, 2024 13:23

It builds

ee01557

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

67aedd4

…to contra-schema

github-actions bot added area: dds Issues related to distributed data structures area: dds: tree base: main PRs targeted against main branch labels Oct 22, 2024

CraigMacomber mentioned this pull request Oct 22, 2024

tree: Cleanup related to contravariant input types #22875

Merged

CraigMacomber added 2 commits October 22, 2024 17:23

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

68c41f7

…to contra-schema

Fix build

8178c40

github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct public api change Changes to a public API labels Oct 23, 2024

CraigMacomber added 2 commits October 22, 2024 17:38

cleanup

0012eab

cleanup

ed6df2d

CraigMacomber commented Oct 23, 2024

View reviewed changes

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

msfluid-bot reviewed Oct 23, 2024

View reviewed changes

Update enum examples

4b328bd

Josmithr reviewed Oct 23, 2024

View reviewed changes

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

Docs

309cf2f

github-actions bot added the changeset-present label Oct 23, 2024

CraigMacomber added 2 commits October 23, 2024 12:17

Fix up docs and unsafe types

d86e122

Merge branch 'main' into contra-schema

847696a

CraigMacomber marked this pull request as ready for review October 23, 2024 19:26

CraigMacomber requested review from a team as code owners October 23, 2024 19:26

tylerbutler approved these changes Oct 23, 2024

View reviewed changes

.changeset/shy-dots-prove.md Outdated Show resolved Hide resolved

.changeset/shy-dots-prove.md Outdated Show resolved Hide resolved

.changeset/shy-dots-prove.md Outdated Show resolved Hide resolved

.changeset/shy-dots-prove.md Outdated Show resolved Hide resolved

Apply suggestions from code review

038a4e3

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