Skip to content
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

Tree: Misc schema related cleanup and fix #11827

Merged
merged 2 commits into from Sep 6, 2022

Conversation

CraigMacomber
Copy link
Contributor

Description

Misc cleanups and fix from #11589 (and some extra cleanup to the Json domain schema).

Fix is that StoredSchemaRepository did not actually use the schema data passes into it.

@CraigMacomber CraigMacomber requested review from a team as code owners September 6, 2022 17:40
@github-actions github-actions bot added area: dds Issues related to distributed data structures public api change Changes to a public API base: main PRs targeted against main branch labels Sep 6, 2022
packages/dds/tree/src/tree/types.ts Outdated Show resolved Hide resolved
@@ -29,64 +29,40 @@ const jsonTypes: Set<TreeSchemaIdentifier> = new Set();

const json: NamedTreeSchema[] = [];

export const jsonObject: NamedTreeSchema = {
export const jsonObject: NamedTreeSchema = namedTreeSchema({
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We probably don't need the type annotation anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered removing them. Since this schema kind of serves as an example, I left the overly explicit types think they might make it slightly easier to understand and navigate to the relevant types. Could really go either way though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, I don't have a strong opinion either way.

Co-authored-by: Jenn <jennle@microsoft.com>
@CraigMacomber CraigMacomber merged commit f1728bb into microsoft:main Sep 6, 2022
@CraigMacomber CraigMacomber deleted the proxy2 branch September 6, 2022 21:01
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

This commit is queued for merging with the next branch! Please ignore this PR for now. Contact @microsoft/fluid-cr-infra for help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds Issues related to distributed data structures base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants