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

Remove redundant members of ISharedObject #10076

Merged
merged 2 commits into from
May 3, 2022

Conversation

CraigMacomber
Copy link
Contributor

Deduplicate the redundant methods ISharedObject declares and also inherits from IChannel.

@github-actions github-actions bot added area: dds Issues related to distributed data structures area: runtime Runtime related issues public api change Changes to a public API base: main PRs targeted against main branch labels Apr 27, 2022
@@ -88,11 +84,13 @@ export function serializeHandles(value: any, serializer: IFluidSerializer, bind:
// @public
export abstract class SharedObject<TEvent extends ISharedObjectEvents = ISharedObjectEvents> extends SharedObjectCore<TEvent> {
constructor(id: string, runtime: IFluidDataStoreRuntime, attributes: IChannelAttributes);
// (undocumented)
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 have been unable to fix that these are now considered "undocumented". I also do not know how to see and generated documentation produced from this code, nor do the inheritDoc tags work in vs code, so I have no way to test or use the generated documentation.

Copy link
Contributor Author

@CraigMacomber CraigMacomber May 3, 2022

Choose a reason for hiding this comment

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

As far as I can tell, this file is the only thing currently consuming the .api.json file.

It's unclear to me if this documentation being omitted here is a good thing or a bad thing:
The entire entry for these methods is omitted from the .api.json file. This could mean that it detected that the base method from the interface is sufficient to describe it, which I think would be a good thing. Alternativity it could mean that I broke the documentation.

Either way, we don't have anything that consumes uses that documentation, and vs-code intelisense is broken due to there being an inherit doc comment in the first place, so that did not regress.

Thus the one thing I use (vs code intelisense) is unchanged (still broken), and the rest is ambiguously better or worse. Thus I guess it should be fine to make this change?

Copy link
Member

Choose a reason for hiding this comment

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

This is probably because the inheritDoc reference now crosses a package boundary, which doesn't work today: microsoft/rushstack#1195

If an @inheritdoc tag refers to an external package, API Extractor does not process it. But API Documenter will process it, if it can find the target in one of its .api.json files.

Copy link
Member

Choose a reason for hiding this comment

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

The entire entry for these methods is omitted from the .api.json file. This could mean that it detected that the base method from the interface is sufficient to describe it, which I think would be a good thing.

The API report should not include inherited members, so this is correct.

@CraigMacomber CraigMacomber marked this pull request as ready for review May 3, 2022 18:27
@CraigMacomber CraigMacomber requested review from a team as code owners May 3, 2022 18:27
@CraigMacomber CraigMacomber merged commit b1da9ee into microsoft:main May 3, 2022
sonalivdeshpande pushed a commit to sonalivdeshpande/FluidFramework that referenced this pull request May 5, 2022
sonalivdeshpande pushed a commit that referenced this pull request May 15, 2022
sonalivdeshpande pushed a commit to sonalivdeshpande/FluidFramework that referenced this pull request May 16, 2022
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 area: runtime Runtime related issues 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.

2 participants