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

Expose SchemaFactoryAlpha as an alpha API #23362

Merged
merged 6 commits into from
Dec 19, 2024

Conversation

Abe27342
Copy link
Contributor

Description

Exposes SchemaFactoryAlpha and associated additional options on object and objectRecursive to tree's alpha API surface. This lights up the feature implemented in #23192, so I have also added a changeset with some user-facing guidance on that feature.

@Copilot Copilot bot review requested due to automatic review settings December 18, 2024 21:18
@Abe27342 Abe27342 requested review from a team as code owners December 18, 2024 21:18

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 14 changed files in this pull request and generated no comments.

Files not reviewed (9)
  • packages/dds/tree/api-report/tree.legacy.public.api.md: Evaluated as low risk
  • packages/dds/tree/src/index.ts: Evaluated as low risk
  • packages/dds/tree/api-report/tree.beta.api.md: Evaluated as low risk
  • packages/dds/tree/api-report/tree.legacy.alpha.api.md: Evaluated as low risk
  • packages/dds/tree/api-report/tree.alpha.api.md: Evaluated as low risk
  • packages/dds/tree/api-report/tree.public.api.md: Evaluated as low risk
  • .changeset/moody-pandas-fall.md: Evaluated as low risk
  • packages/dds/tree/src/test/shared-tree/schematizingTreeView.spec.ts: Evaluated as low risk
  • packages/dds/tree/src/simple-tree/index.ts: Evaluated as low risk
Comments suppressed due to low confidence (1)

packages/dds/tree/src/simple-tree/api/schemaFactoryAlpha.ts:90

  • The method object is being called within objectRecursive but it is not clear if object supports the same options parameter. Ensure that object method can handle SchemaFactoryObjectOptions correctly.
return this.object(
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: tree changeset-present public api change Changes to a public API base: main PRs targeted against main branch and removed area: dds Issues related to distributed data structures public api change Changes to a public API area: dds: tree changeset-present labels Dec 18, 2024
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: tree area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct changeset-present public api change Changes to a public API labels Dec 18, 2024
@@ -103,6 +103,8 @@ export function schemaFromValue(value: TreeValue): TreeNodeSchema {

/**
* Options when declaring an {@link SchemaFactory.object|object node}'s schema
*
* @alpha
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Question about the documentation on the property below. The first line reads:

Opt in to viewing documents which have unknown optional fields for this object's identifier, i.e. optional fields
in the document schema which are not present in this object schema declaration.

I'm not sure what "identifier" means in this context. Should this instead just say something like:

Allow nodes typed with this object node schema to contain optional fields that are not present in the schema declaration.

?

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 like that way of putting it. identifier refers to the schema's identifier, but it's not particularly necessary to mention that here.

Copy link
Contributor

@Josmithr Josmithr left a comment

Choose a reason for hiding this comment

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

Left a few suggestions but otherwise looks great to me! And lines up very well with unrelated schema factory functionality I'm working on.

You mentioned that API-Extractor was complaining about these changes earlier, but no longer seems to be? Let me know if I can help investigate anything there. But assuming it continues to be stable, looks good to me!

@Abe27342
Copy link
Contributor Author

Left a few suggestions but otherwise looks great to me! And lines up very well with unrelated schema factory functionality I'm working on.

You mentioned that API-Extractor was complaining about these changes earlier, but no longer seems to be? Let me know if I can help investigate anything there. But assuming it continues to be stable, looks good to me!

Yeah, I'm not totally sure it was actually an API-extractor issue, it just surfaced that way. I think the original problem I was seeing was typescript emitting 'any' which down the line caused problems, but it's plausible that when I last tried this I had missed something subtle (previous PR had a lot of changes) and when reimplementing this from a fresh state I didn't make the same mistake.

Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170138 links
    1596 destination URLs
    1825 URLs ignored
       0 warnings
       0 errors


@Abe27342 Abe27342 enabled auto-merge (squash) December 19, 2024 15:30
@Abe27342 Abe27342 merged commit 2406e00 into microsoft:main Dec 19, 2024
32 checks passed
@Abe27342 Abe27342 deleted the expose-schemafactoryalpha branch December 19, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: tree area: dds Issues related to distributed data structures area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct base: main PRs targeted against main branch changeset-present public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants