Add override to processMessagesCore implementations, and remove inheritDoc#26526
Conversation
| * {@inheritDoc @fluidframework/shared-object-base#SharedObject.processMessagesCore} | ||
| */ | ||
| protected processMessagesCore(messagesCollection: IRuntimeMessageCollection): void { | ||
| protected override processMessagesCore(messagesCollection: IRuntimeMessageCollection): void { |
There was a problem hiding this comment.
This is for TaskManagerClass which is exported as @legacy+beta and therefore until tooling is fixed inheritDoc is needed to get doc generation, at per last discussion of this 3/18/2025.
It would be nice if the api report noted that this method is now undocumented OR if we checked in the generated doc json file that shows what will be relayed for documentation generation so that we could see this delta.
All other cases for exported classes need similar keeping. (Also, better if we just stop exporting classes - especially sealed ones.)
cc: @Josmithr
There was a problem hiding this comment.
I don't understand. Release tag levels have no impact on whether or not something gets API documentation. API-Extractor does not automatically inherit documentation in any circumstances. So, this change does have the effect of removing explicit method documentation from the resulting API docs, but this is a trade-off we've made elsewhere (prioritizing intellisense over API docs). And it doesn't have anything to do with the release tagging.
Also note that type-level inheritance is captured by the API docs, so a reader can navigate to the base declaration (in this case, SharedObjectBase) to view the documentation. It's certainly a worse experience overall, but we are unfortunately required to make these trade-offs until one of the following:
- Intellisense actually supports TSDoc (rather than JSDoc)
- API-Extractor is updated to persist inheritance information more granularly
- We do hacky things in
api-markdown-documenterto try to re-derive this information dynamically.
There was a problem hiding this comment.
Can we get https://github.com/microsoft/FluidFramework/wiki/TSDoc-Guidelines#inheritDoc updated indicate when inheritDoc should be omitted to let the inheritance happen implicitly? Sounds like maybe we need it for package exported non-internal APIs? Or maybe just those APIs when crossing between packages?
There was a problem hiding this comment.
@Josmithr, release tags are relevant because that indicates a non-internal use. We don't generate docs for @internal and will just rely on IntelliSense (that is happier without @inheritDoc).
There was a problem hiding this comment.
@jason-ha What @internal types are involved here?
There was a problem hiding this comment.
Can we get https://github.com/microsoft/FluidFramework/wiki/TSDoc-Guidelines#inheritDoc updated indicate when inheritDoc should be omitted to let the inheritance happen implicitly? Sounds like maybe we need it for package exported non-internal APIs? Or maybe just those APIs when crossing between packages?
I'll file an item for myself to update the guidance for that and @override.
There was a problem hiding this comment.
I don't know. I really only looked at this class. I do know that we have inheritDoc in places where it is detrimental.
There was a problem hiding this comment.
if the api report noted that this method is now undocumented
It does. I just failed to include the API report updates. That has been corrected.
That said, Tyler is working to make all these classes internal currently, so they will soon be out of the API reports :). We do not actually intend to support any non-internal implanters or callers of this API, but a few soon to be gone exports are keeping it in the legacy API at the moment.
There was a problem hiding this comment.
Maybe we don't have API reports for task-manager. I don't see a now undocumented note for this in an api.md file.
There was a problem hiding this comment.
Also, I don't have an issue with not having a comment for this. Just want to make sure we know what we are doing.
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
There was a problem hiding this comment.
Pull request overview
Updates DDS implementations of SharedObject.processMessagesCore to use TypeScript’s override keyword and removes {@inheritDoc ...processMessagesCore} blocks that interfere with IntelliSense, with corresponding API report updates where applicable.
Changes:
- Add
protected override processMessagesCore(...)across DDS implementations (including tests and experimental DDSs). - Remove
{@inheritDoc @fluidframework/shared-object-base#SharedObject.processMessagesCore}doc blocks from overrides. - Update API report files to reflect the documentation/metadata changes (now showing
// (undocumented)forprocessMessagesCorein a few exported classes).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/test/test-end-to-end-tests/src/test/summarization/summarizeIncrementallySubDds.spec.ts | Add override to test DDS processMessagesCore implementations. |
| packages/dds/task-manager/src/taskManager.ts | Remove inheritDoc for processMessagesCore and add override. |
| packages/dds/shared-summary-block/src/sharedSummaryBlock.ts | Remove inheritDoc and add override for processMessagesCore. |
| packages/dds/shared-object-base/src/test/sharedObject.spec.ts | Add override in test classes implementing processMessagesCore. |
| packages/dds/sequence/src/sequence.ts | Remove inheritDoc and add override for processMessagesCore. |
| packages/dds/register-collection/src/consensusRegisterCollection.ts | Remove inheritDoc and add override for processMessagesCore. |
| packages/dds/register-collection/api-report/register-collection.legacy.beta.api.md | API report updated to reflect undocumented processMessagesCore. |
| packages/dds/pact-map/src/pactMap.ts | Remove inheritDoc and add override for processMessagesCore. |
| packages/dds/ordered-collection/src/consensusOrderedCollection.ts | Remove inheritDoc and add override for processMessagesCore. |
| packages/dds/ordered-collection/api-report/ordered-collection.legacy.beta.api.md | API report updated to reflect undocumented processMessagesCore. |
| packages/dds/matrix/src/matrix.ts | Remove inheritDoc and add override for processMessagesCore. |
| packages/dds/map/src/map.ts | Remove inheritDoc block above existing override processMessagesCore. |
| packages/dds/map/src/directory.ts | Remove inheritDoc block above existing override processMessagesCore. |
| packages/dds/legacy-dds/src/signal/sharedSignal.ts | Remove inheritDoc and add override for processMessagesCore. |
| packages/dds/legacy-dds/src/array/sharedArray.ts | Remove inheritDoc and add override for processMessagesCore. |
| packages/dds/ink/src/ink.ts | Remove inheritDoc and add override for processMessagesCore. |
| packages/dds/counter/src/counter.ts | Remove inheritDoc and add override for processMessagesCore. |
| packages/dds/cell/src/cell.ts | Remove inheritDoc and add override for processMessagesCore. |
| experimental/dds/tree/src/SharedTree.ts | Remove inheritDoc and add override for processMessagesCore. |
| experimental/dds/tree/api-report/experimental-tree.alpha.api.md | API report updated to reflect undocumented processMessagesCore. |
| experimental/dds/ot/ot/src/ot.ts | Remove inheritDoc and add override for processMessagesCore. |
| experimental/PropertyDDS/packages/property-dds/src/propertyTree.ts | Remove inheritDoc and add override for processMessagesCore. |
Description
Add
overrideto processMessagesCore implementations, and remove inheritDoc which breaks intelisense and seems to offer little to no value.Reviewer Guidance
The review process is outlined on this wiki page.
I think https://github.com/microsoft/FluidFramework/wiki/TSDoc-Guidelines#override might need to be corrected/updated. I made this PR based on what I think our standards should be, but its draft for now as we don't have a clear policy.