-
Notifications
You must be signed in to change notification settings - Fork 549
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
base: main
Are you sure you want to change the base?
feat: customizeSchemaTyping #23084
Conversation
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
"section": tree | ||
--- | ||
|
||
Disallow some invalid and unsafe ObjectNode field assignments at compile time |
There was a problem hiding this comment.
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">(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>()), |
There was a problem hiding this comment.
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; | ||
}); | ||
|
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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).
There was a problem hiding this 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 Name | Baseline coverage | PR coverage | Coverage 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 Name | Baseline coverage | PR coverage | Coverage 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!!
This change includes some links which depend on #23533 to link the alpha APIs. |
The above PR has been merged :) |
Co-authored-by: Tommy Brosman <tbrosman@microsoft.com>
🔗 Found some broken links! 💔 Run a link check locally to find them. See linkcheck output
|
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! |
Still pending |
Description
Add
customizeSchemaTyping
.Status: Proof of concept / Proposal.
Before this could be mergeed it would need:
Breaking Changes
TODO
Reviewer Guidance
The review process is outlined on this wiki page.