Skip to content

feat(aqueduct): Expose ITree & ISharedObject type for TreeDataObject's SharedTree #24684

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

Closed

Conversation

jikim-msft
Copy link
Contributor

Description

This PR exposes ITree & ISharedObject type for the TreeDataObject.sharedTree property.

Relevant PR & Discussion

#24523 (comment)

@jikim-msft jikim-msft requested review from Copilot and Josmithr and removed request for Copilot May 22, 2025 00:47
@github-actions github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct base: main PRs targeted against main branch and removed area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct labels May 22, 2025

/**
* Gets the underlying {@link @fluidframework/tree#ITree | tree}.
*/
public get sharedTree(): ITree {
public get sharedTree(): ITree & ISharedObject {
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems incorrect. the docs on the type say:

  • Base interface for shared objects from which other interfaces extend.
  • @remarks
  • This interface is not intended to be implemented outside this repository:
  • implementers should migrate to using an existing implementation instead.
  • @privateRemarks
  • Implemented by {@link SharedObjectCore}.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems fine internally, but i don't think the public getter should expose it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @anthony-murphy, after going through the doc more thoroughly, I agree with your point. Will close this PR. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

i think the change was fine besides the public exposure on the getter. using it internally made the bind cast better which i thought was fine.

@jikim-msft jikim-msft closed this May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants