-
Notifications
You must be signed in to change notification settings - Fork 550
feat(tree): Update experimental Table Schema APIs to support adding additional properties to Column/Row Schema #24463
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
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.
Pull Request Overview
This PR updates the experimental Table Schema APIs to support additional properties for Column and Row schemas by introducing new props schemas and updating the tests and internal type signatures accordingly. Key changes include:
- Introducing ColumnProps and RowProps classes for schema definitions.
- Updating the createColumn, createRow, and createTable implementations and tests to require and handle a new “props” field.
- Adjusting tests to consistently include a props object when creating schema instances.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
packages/dds/tree/src/test/tableSchema.spec.ts | Updates to test cases to include props for columns and rows. |
packages/dds/tree/src/tableSchema.ts | Adjustments to schema creation functions and type signatures to support props. |
examples/utils/import-testing/src/test/importer.spec.ts | Updates to integration tests to utilize the new props-based API. |
Comments suppressed due to low confidence (2)
packages/dds/tree/src/test/tableSchema.spec.ts:77
- Consider adding tests for cases where the props field is omitted to ensure the schema behaves as expected when props are not provided, which will help ensure backward compatibility.
treeView.initialize(new Table({
packages/dds/tree/src/tableSchema.ts:470
- Add inline documentation clarifying that using 'inputSchemaFactory.null' as the default for props is a temporary workaround, and consider extracting this logic into a helper function to improve readability.
const column = columnSchema ?? createColumn(inputSchemaFactory, inputSchemaFactory.null);
@@ -180,31 +202,34 @@ export namespace TableSchema { | |||
const rowFields = { | |||
id: schemaFactory.identifier, | |||
cells: schemaFactory.map("Row.cells", cellSchema), | |||
} as const satisfies Record<string, ImplicitFieldSchema>; |
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.
We discovered that the satisfies
clause here was actually resulting in different (and incorrect) typing to be applied to rowFields
. This appears to be a TypeScript bug. But since this extra check isn't really necessary for type-safety in this function, it can be safely removed.
This PR also decouples Row and Column schema. Row previously took schema-specific Column as input so it could offer APIs that took in Column nodes. These have been replaced with 2 overloads each: 1 that takes an ID (resolving an existing TODO) and one that takes in an IColumn.
Notes:
.d.ts
files when I tried this previously. I will continue investigating and make this change in a subsequent PR.SchemaFactory.null
.