Skip to content

feat: customizeSchemaTyping #23084

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

Draft
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

CraigMacomber
Copy link
Contributor

@CraigMacomber CraigMacomber commented Nov 13, 2024

Description

Add customizeSchemaTyping.

Status: Proof of concept / Proposal.

Before this could be mergeed it would need:

  • Conceptual review and approval
  • Cleanup of naming, docs and exports
  • API review
  • Attempt to update (and test) recursive type variants to use this pattern.
  • More testing

Breaking Changes

TODO

Reviewer Guidance

The review process is outlined on this wiki page.

@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: tree area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct changeset-present public api change Changes to a public API base: main PRs targeted against main branch labels Nov 13, 2024
"section": tree
---

Disallow some invalid and unsafe ObjectNode field assignments at compile time
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 PR builds upon #23053 . This changeset will probably need to be rewritten based on the major changes.


it("customized narrowing", () => {
class Specific extends schema.object("Specific", {
s: customizeSchemaTyping(schema.string).simplified<"foo" | "bar">(),
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 shows customizeSchemaTyping being used to implement a previously requested feature of narrowing leaf types.

It can also be used for branding of lead types, which would be even safer from a soundness perspective.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks cool

it("mutable static registry, safer editing API", () => {
const ItemTypes: ItemSchema[] = [];
class Container extends sf.object("Container", {
child: sf.optional(customizeSchemaTyping(ItemTypes).simplified<Item>()),
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 file shows several ways to do open polymorphism / schema child dependency injection. This one uses customizeSchemaTyping, and makes everything just work.

container.child = TextItem.default();
container.child = undefined;
});

Copy link
Contributor

Choose a reason for hiding this comment

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

I would love to try something like this:

interface A {
  type: "a";
  foo: string;
}
interface B {
  type: "b";
  bar: number;
}
interface C {
  easy: number
}
type Combo = A | B;
interface Root {
  faz: Combo;
  c: C;
}

class SyncedRoot extends sf.object(...) implements Root {}
...

const a: A = { ... }
const root: Root = {...}
const syncedRoot = new SyncedRoot(root);
syncedRoot.faz = new SyncedA(a);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cases like that where Combo is explicitly referring the types in the union work fine, even without this change. The problem that is being addressed and demoed in this file is "open "polymorphism where you need to somehow express "Combo" without referencing the actual members in it leaving the implicit union "open" for extension (in such a setup the implementation refer to what they are implementing, in this case "Combo", like if Combo and SyncCombo were interfaces things could implement, not a union of types).

Copy link
Collaborator

@msfluid-bot msfluid-bot left a comment

Choose a reason for hiding this comment

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

Code Coverage Summary

↓ packages.dds.tree.src.simple-tree.api:
Line Coverage Change: -0.05%    Branch Coverage Change: No change
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 88.76% 88.76% → No change
Line Coverage 82.35% 82.30% ↓ -0.05%
↑ packages.dds.tree.src.simple-tree:
Line Coverage Change: 0.15%    Branch Coverage Change: 0.02%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 94.07% 94.09% ↑ 0.02%
Line Coverage 97.23% 97.38% ↑ 0.15%

Baseline commit: e603044
Baseline build: 307393
Happy Coding!!

Code coverage comparison check passed!!

@CraigMacomber
Copy link
Contributor Author

This change includes some links which depend on #23533 to link the alpha APIs.

@Josmithr
Copy link
Contributor

This change includes some links which depend on #23533 to link the alpha APIs.

The above PR has been merged :)

Copy link
Contributor

🔗 Found some broken links! 💔

Run a link check locally to find them. See
https://github.com/microsoft/FluidFramework/wiki/Checking-for-broken-links-in-the-documentation for more information.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

http://localhost:3000/docs/data-structures/tree
- (36:97) 'the Shar..' => http://localhost:3000/docs/api/tree (HTTP 404)

http://localhost:3000/docs/data-structures/tree/
- (36:97) 'the Shar..' => http://localhost:3000/docs/api/tree (HTTP 404)

http://localhost:3000/docs/data-structures/tree/schema-definition
- (30:128) 'SharedTr..' => http://localhost:3000/docs/api/tree (HTTP 404)

http://localhost:3000/docs/start/tree-start
- (44:4) 'the API ..' => http://localhost:3000/docs/api/tree/schemafactory-class (HTTP 404)
- (61:7) 'the API' => http://localhost:3000/docs/api/tree/treechangeevents-interface (HTTP 404)


Stats:
  162291 links
    1317 destination URLs
    1549 URLs ignored
       0 warnings
       3 errors

 ELIFECYCLE  Command failed with exit code 1.

Copy link
Contributor

This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

@CraigMacomber
Copy link
Contributor Author

Still pending

@CraigMacomber CraigMacomber reopened this Jun 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: tree area: dds Issues related to distributed data structures area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct base: main PRs targeted against main branch changeset-present public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants