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.

Sign up for GitHub

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

docs: update/correct comments for submitSignalFn #23403

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+20 −7
Open

docs: update/correct comments for submitSignalFn #23403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions packages/common/container-definitions/src/runtime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,15 @@ export interface IBatchMessage {
* loader layer.
* It gets passed into the {@link (IRuntimeFactory:interface).instantiateRuntime} call.
*
* @privateremarks
* @privateRemarks
* Only include members on this interface if you intend them to be consumed/called from the runtime layer.
*
* TODO: once `@alpha` tag is removed, `unknown` should be removed from submitSignalFn.
* See {@link https://dev.azure.com/fluidframework/internal/_workitems/edit/7462}
* TODO: once `@internal` or `@system`, `submitSignalFn`'s `contents` `unknown` type should be replaced with
* {@link @fluidframework/core-interfaces#ISignalEnvelope}.
* See {@link https://dev.azure.com/fluidframework/internal/_workitems/edit/7462}.
*
* To get `IContainerContext` internal, likely externally exposed uses will need
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we're on a track to make IContainerContext internal, but rather I think an equivalent eventually may become part of the public API. This is (currently) required for the extensibility point of a customer-defined IRuntimeFactory, which is actually a useful tool. The interfaces could do with a lot of improvements, but the core concept seems valuable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting crossroads. ISignalEnvelope is internal and the required type to pass to submitSignalFn.
I guess the TODO is to remove submitSignalFn from this interface and offer an alternative if needed. Maybe Presence notifications would be sufficient.

* to be replaced with a branded or otherwise erased type.
*
* @legacy
* @alpha
Expand Down Expand Up @@ -173,6 +177,9 @@ export interface IContainerContext {
summaryOp: ISummaryContent,
referenceSequenceNumber?: number,
) => number;
/**
* @param content - Expected to be of type {@link @fluidframework/core-interfaces/internal#ISignalEnvelope}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a semantic description of submitSignalsFn while we're here? 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

(Feel free to close with no action - I just have a compulsive need to ask for more documentation 😜)

*/
readonly submitSignalFn: (contents: unknown, targetClientId?: string) => void;
readonly disposeFn?: (error?: ICriticalContainerError) => void;
readonly closeFn: (error?: ICriticalContainerError) => void;
Expand Down
5 changes: 4 additions & 1 deletion packages/loader/container-loader/src/container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2378,7 +2378,10 @@ export class Container
this.emit("op", message);
}

// unknown should be removed once `@alpha` tag is removed from IContainerContext
/**
* `unknown` should be removed once `IContainerContext` is `@internal` or `@system`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit (to conform to the expectation that the summary portion of a TSDoc comment is the semantic description of the thing being documented):

Suggested change
* `unknown` should be removed once `IContainerContext` is `@internal` or `@system`.
* @privateRemarks `unknown` should be removed once `IContainerContext` is `@internal` or `@system`.

* @see {@link https://dev.azure.com/fluidframework/internal/_workitems/edit/7462}
*/
private submitSignal(content: unknown | ISignalEnvelope, targetClientId?: string): void {
this._deltaManager.submitSignal(JSON.stringify(content), targetClientId);
}
Expand Down
7 changes: 5 additions & 2 deletions packages/loader/container-loader/src/containerContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,12 @@ export class ContainerContext implements IContainerContext {
) => number,

/**
* `unknown` should be removed once `@alpha` tag is removed from IContainerContext
* `unknown` should be removed once `IContainerContext` is `@internal` or `@system`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same feedback as https://github.com/microsoft/FluidFramework/pull/23403/files#r1926044765 - this note should move to the @privateRemarks block.

* @see {@link https://dev.azure.com/fluidframework/internal/_workitems/edit/7462}
* Any changes to submitSignalFn `content` should be checked internally by temporarily changing IContainerContext and removing all `unknown`s
*
* @privateRemarks
* Any changes to submitSignalFn `content` should be checked internally by temporarily
* changing `IContainerContext` (replacing/removing all `unknown`s).
*/
public readonly submitSignalFn: (
content: unknown | ISignalEnvelope,
Expand Down
2 changes: 1 addition & 1 deletion packages/runtime/container-runtime/src/containerRuntime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1217,7 +1217,7 @@ export class ContainerRuntime
referenceSequenceNumber?: number,
) => number;
/**
* Do not call directly - use submitAddressesSignal
* Do not call directly - use {@link ContainerRuntime.submitEnvelopedSignal}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit

Suggested change
* Do not call directly - use {@link ContainerRuntime.submitEnvelopedSignal}
* @remarks Do not call directly - use {@link ContainerRuntime.submitEnvelopedSignal}

*/
private readonly submitSignalFn: (content: ISignalEnvelope, targetClientId?: string) => void;
public readonly disposeFn: (error?: ICriticalContainerError) => void;
Expand Down
Loading