Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tree: Improve non-schema aware typing #22763

Merged
merged 29 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
c6bff5f
Improve non-schema aware typing
CraigMacomber Oct 8, 2024
be09116
Apply suggestions from code review
CraigMacomber Oct 9, 2024
0b3e0f2
Fix API-extractor
CraigMacomber Oct 9, 2024
2094876
Update .vscode/settings.json
CraigMacomber Oct 9, 2024
54b9730
Merge branch 'main' into BetterTypes2
CraigMacomber Oct 9, 2024
24c9042
Apply suggestions from code review
CraigMacomber Oct 9, 2024
56b1da9
more privateRemarks
CraigMacomber Oct 9, 2024
11b9881
work on docs for TreeFieldFromImplicitField
CraigMacomber Oct 9, 2024
4e0913d
Update packages/dds/tree/src/simple-tree/arrayNode.ts
CraigMacomber Oct 9, 2024
c768c55
Add ReadonlyArrayNode limitation note
CraigMacomber Oct 9, 2024
8d4c75b
Merge branch 'BetterTypes2' of https://github.com/CraigMacomber/Fluid…
CraigMacomber Oct 9, 2024
b66a64a
typing tweaks
CraigMacomber Oct 9, 2024
9525948
Update packages/dds/tree/src/simple-tree/schemaTypes.ts
CraigMacomber Oct 9, 2024
1d82b30
Merge branch 'main' into BetterTypes2
CraigMacomber Oct 9, 2024
b51af25
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
CraigMacomber Oct 10, 2024
10d0fbe
Update changeset. More docs and tests.
CraigMacomber Oct 10, 2024
be3eb41
Merge branch 'main' into BetterTypes2
CraigMacomber Oct 11, 2024
2ff6286
Fix build
CraigMacomber Oct 11, 2024
610b22e
Merge branch 'main' into BetterTypes2
CraigMacomber Oct 11, 2024
a0f848d
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
CraigMacomber Oct 21, 2024
3476f1c
Apply suggestions from code review
CraigMacomber Oct 21, 2024
2f5be79
Apply suggestions from code review
CraigMacomber Oct 21, 2024
db7dfea
Apply suggestions from code review
CraigMacomber Oct 21, 2024
6a1a891
Apply suggestions from code review
CraigMacomber Oct 22, 2024
68ea5d8
tweak doc comment.
CraigMacomber Oct 22, 2024
15257c6
Edit changeset for clarity
CraigMacomber Oct 22, 2024
a360253
Merge branch 'main' into BetterTypes2
CraigMacomber Oct 22, 2024
89018a7
Merge branch 'main' into BetterTypes2
CraigMacomber Oct 22, 2024
0160982
Merge branch 'main' into BetterTypes2
CraigMacomber Oct 22, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 123 additions & 0 deletions .changeset/shiny-swans-relax.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
---
"fluid-framework": minor
"@fluidframework/tree": minor
---
---
"section": tree
---

Improve typing when exact TypeScript type for a schema is not provided

The Tree APIs are designed to be used in a strongly typed way, with the full TypeScript type for the schema always being provided.
Due to limitations of the TypeScript language, there was not a practical way to prevent less descriptive types, like `TreeNodeSchema` or `ImplicitFieldSchema` from being used where the type of a specific schema was intended.
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
Code which does this will encounter several issues with tree APIs, and this change fixes some of those issues.
This change mainly fixes that `NodeFromSchema<TreeNodeSchema>` used to return `unknown` and now returns `TreeNode | TreeLeafValue`.
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved

This change by itself seems mostly harmless, as it just improves the precision of the typing in this one edge case.
Unfortunately there are other typing bugs which complicate the situation, resulting in APIs for inserting data into the tree to also behave poorly when given non-specific types like `TreeNodeSchema`.
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
These APIs include cases like `TreeView.initialize`.

This incorrectly allowed some usage like taking a type erased schema and initial tree pair, and creating a view of type `TreeView<ImplicitFieldSchema>` then initializing a it.
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
With the typing being partly fixed, some unsafe inputs are still allowed when trying to initialize such a view, but some are now prevented.

While this use-case of modifying in code not strongly typed by the exact schema, was not intended to be supported,
Copy link
Contributor

Choose a reason for hiding this comment

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

This clause is a bit hard to parse, I think there might be a typo here, but I'm not sure what the intention was.

it did mostly work, and has some real use-cases (like tests looping over test data pairs of schema and tree data).
To help mitigate the impact of this change, some experimental `@alpha` APis have been introduced.

Before this change:

```typescript
import { TinyliciousClient } from "@fluidframework/tinylicious-client";
import {
SchemaFactory,
SharedTree,
TreeViewConfiguration,
type TreeNodeSchema,
} from "fluid-framework";

// Create a ITree instance
const tinyliciousClient = new TinyliciousClient();
const { container } = await tinyliciousClient.createContainer({ initialObjects: {} }, "2");
const tree = await container.create(SharedTree);

const schemaFactory = new SchemaFactory("demo");

// Bad: This loses the schema aware type information. `: TreeNodeSchema` should be omitted to preserve strong typing.
const schema: TreeNodeSchema = schemaFactory.array(schemaFactory.number);
const config = new TreeViewConfiguration({ schema });

// This view is types as `TreeView<TreeNodeSchema>`, which does not work well since its missing the actual schema type information.
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
const view = tree.viewWith(config);
// Root is typed as `unknown` allowing invalid assignment operations.
view.root = "invalid";
view.root = {};
// Since all assignments are allowed, valid ones still work:
view.root = [];
```

After this change:


```typescript
// Root is now typed as `TreeNode | TreeLeafValue`, still allowing some invalid assignment operations.
// In the future this should be prevented as well, since the type of the setter in this case should ne `never`
noencke marked this conversation as resolved.
Show resolved Hide resolved
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
view.root = "invalid";
// This no longer compiles:
view.root = {};
// This also longer compiles:
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
view.root = [];
```

For code which wants to continue using a unsafe API which can result in runtime errors if the data does not follow the schema, a new alternative has been added to address this use-case. A special type `UnsafeUnknownSchema` can no be used to opt into allowing all valid trees to be provided.
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
Note that this leaves ensuring the data is in schema up to the user.
For now these adjusted APIs can be accessed by casting the view to `TreeViewAlpha<UnsafeUnknownSchema>`.
If stabilized this option wil be added to `TreeView` directly.
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved

```typescript
const viewAlpha = view as TreeViewAlpha<UnsafeUnknownSchema>;
viewAlpha.initialize([]);
viewAlpha.root = [];
```

Additionally this seems to have negatively impacted co recursive schema which declare an corecursive array as the first schema in the co-recursive cycle.
CraigMacomber marked this conversation as resolved.
Show resolved Hide resolved
Like the TypeScript language our schema system is built on, we don't guarantee exactly which recursive type will compile, but will do our best to ensure useful recursive schema can be created easily.
In this case a slight change may be required to some recursive schema to get them to compile again:

For example this schema used to compile:


```typescript
class A extends sf.arrayRecursive("A", [() => B]) {}
{
type _check = ValidateRecursiveSchema<typeof A>;
}
// Used to work, but breaks in this update.
class B extends sf.object("B", { x: A }) {}
```

But now you must use the recursive functions like `objectRecursive` for types which are co-recursive with an array in some cases.
In our example, it can be fixed as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I always just assumed this is how you were supposed to do it anyway. I don't think we made that explicitly clear anywhere but it makes sense, at least to me. The fact that either of the other two ways ever worked or still work feels like a hack to me. So, I don't feel that bad about this "breaking change", but I'm just one data point.

Copy link
Contributor

@Josmithr Josmithr Oct 21, 2024

Choose a reason for hiding this comment

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

I had the same assumption, and draw the same conclusion as Noah.


```typescript
class A extends sf.arrayRecursive("A", [() => B]) {}
{
type _check = ValidateRecursiveSchema<typeof A>;
}
// Fixed corecursive type, using "Recursive" method variant to declare schema.
class B extends sf.objectRecursive("B", { x: A }) {}
{
type _check = ValidateRecursiveSchema<typeof B>;
}
```

Reversing which type is using the recursive utility also works (shown below).
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the above pattern is our actual recommendation, do we need to note this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was mostly intended to direct people away from relying on this despite it compiling. We don't want people doing what I did in the past and thing they are clever and getting a better devx by using the recursive helpers less since it might break again but differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with removing it if you think that would be better. I partly just included it since it was interesting and should be discussed in this review.

Copy link
Contributor

@Josmithr Josmithr Oct 21, 2024

Choose a reason for hiding this comment

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

Maybe just rephrase it to say something like "Note: while the following pattern may still compile, we recommend using the previous pattern instead"?

We do not recommend relying on this: using the recursive variances for all co-recursive types will help ensure your schema are less likely to be impacted by changes like this in the future.

```typescript
class B extends sf.objectRecursive("B", { x: [() => A] }) {}
{
type _check = ValidateRecursiveSchema<typeof B>;
}
// Works, for now, but not recommended.
class A extends sf.array("A", B) {}
```
1 change: 1 addition & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"cSpell.words": [
"boop",
"contoso",
"corecursive",
"debuggability",
"denormalized",
"endregion",
Expand Down
10 changes: 9 additions & 1 deletion packages/dds/tree/.vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,15 @@
// This extension appears to invoke mocha programmatically, meaning that the enablement of this option in the common
// mocha test config isn't sufficient; it also needs to be enabled here.
"mochaExplorer.nodeArgv": ["--conditions", "allow-ff-test-exports"],
"cSpell.words": ["deprioritized", "endregion", "insertable", "reentrantly", "unhydrated"],
"cSpell.words": [
"contravariantly",
"covariantly",
"deprioritized",
"endregion",
"insertable",
"reentrantly",
"unhydrated",
],

// Enable biome as default formatter, and disable rules that disagree with it
"editor.defaultFormatter": "biomejs.biome",
Expand Down
Loading
Loading