tree: Add schema symbol to TreeNodes to allow NodeKind based narrowing. #22222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
area: dds
Josmithr
reviewed
Aug 15, 2024
Josmithr
reviewed
Aug 15, 2024
Josmithr
reviewed
Aug 15, 2024
Co-authored-by: Joshua Smithrud <[email protected]>
Josmithr
reviewed
Aug 15, 2024
| function getKeys(node: TreeNode & WithType<string, NodeKind.Map | NodeKind.Object>): string[]; | ||
| function getKeys(node: TreeNode): string[] | number[]; | ||
| function getKeys(node: TreeNode): string[] | number[] { | ||
| const schema = Tree.schema(node); |
There was a problem hiding this comment.
Best practices question, since this example is end-user-facing: should we use a switch here with a default handler that errors on unrecognized node kind? I don't know how likely it is that we will add a fundamentally new node type any time soon, but if we were to do so, the default behavior here may not be what the user wants.
Seems like the switch pattern highlights this potential better. Curious what others think.
Josmithr
reviewed
Aug 15, 2024
packages/dds/tree/src/index.ts
Outdated
| type TreeObjectNodeUnsafe, | ||
| type InsertableTreeNodeFromImplicitAllowedTypesUnsafe, | ||
| type TreeArrayNodeUnsafe, | ||
| type TreeMapNodeUnsafe, | ||
| type InsertableObjectFromSchemaRecordUnsafe, | ||
| type InsertableTreeFieldFromImplicitFieldUnsafe, | ||
| type FieldSchemaUnsafe, | ||
| // System types (not in Internal types for various reasons, like doc links or cannot be named errors). | ||
| getJsonSchema, |
There was a problem hiding this comment.
This function isn't a @system type. Should probably go somewhere else.
There was a problem hiding this comment.
Messed up some copy paste there. Fixed.
Josmithr
approved these changes
Aug 15, 2024
⯅ @fluid-example/bundle-size-tests: +603 Bytes
Baseline commit: 425111e |
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
This was referenced Aug 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
See changeset for details.
This will be used for things like providing object node specific events with strong typing.
This is aimed to provide the functionality attempted in alexvy86#7 to better support node kind specific data in #22043
Reviewer Guidance
The review process is outlined on this wiki page.