-
Notifications
You must be signed in to change notification settings - Fork 535
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
feat: Make NodeFromSchema work in more cases #23089
base: main
Are you sure you want to change the base?
Conversation
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.
⯅ @fluid-example/bundle-size-tests: +245 Bytes
Baseline commit: 3fe3fa5 |
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.
Copilot reviewed 5 out of 12 changed files in this pull request and generated no suggestions.
Files not reviewed (7)
- packages/dds/tree/api-report/tree.legacy.public.api.md: Evaluated as low risk
- packages/framework/fluid-framework/api-report/fluid-framework.public.api.md: Evaluated as low risk
- packages/dds/tree/api-report/tree.legacy.alpha.api.md: Evaluated as low risk
- packages/framework/fluid-framework/api-report/fluid-framework.legacy.alpha.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.public.api.md: Evaluated as low risk
- packages/dds/tree/api-report/tree.beta.api.md: Evaluated as low risk
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
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.
Looks good! Thanks for the comment block describing the ordering choice for the updated template type!
Description
NodeFromSchema now prefers the class based form if given a schema which implements a class based and non class based API.
This shouldn't impact our public APIs (unless someone adds a static create function to their class based schema, in which case this is a fix), but internally we use TreeNodeSchemaBoth in some places, and if you subclass a type implementing TreeNodeSchemaBoth, before this change NodeFromSchema would give the less specialized type from the create function, ignoring the subclassing. This was confusing and has been fixed.
Reviewer Guidance
The review process is outlined on this wiki page.