Inspector v2: Switch some Observable to IReadonlyObservable (#16742)

ryantrem · web-flow · commit 5552c0dc25a0 · 2025-06-12T10:51:58.000-07:00
This PR just switches some contracts to expose `IReadonlyObservable`
instead of `Observable` (for API clarity, and also to simplify some code
since `IReadonlyObservable&lt;T&gt;` is covariant).
Also small change to `IReadonlyObservable&lt;T&gt;` to make `T = unknown` by
default (so for cases where you don't care about the payload, you can
just use `IReadonlyObservable` without any type params).
diff --git a/packages/dev/core/src/Misc/observable.ts b/packages/dev/core/src/Misc/observable.ts
@@ -132,7 +132,7 @@ export class Observer<T> implements IObserver {
 /**
  * An interface that defines the reader side of an Observable (receive notifications).
  */
-export interface IReadonlyObservable<T> {
+export interface IReadonlyObservable<T = unknown> {
     /**
      * Create a new Observer with the specified callback
      * @param callback the callback that will be executed for that Observer
diff --git a/packages/dev/inspector-v2/src/hooks/observableHooks.ts b/packages/dev/inspector-v2/src/hooks/observableHooks.ts
@@ -1,5 +1,5 @@
 // eslint-disable-next-line import/no-internal-modules
-import type { Observable } from "core/index";
+import type { IReadonlyObservable } from "core/index";
 
 import type { ObservableCollection } from "../misc/observableCollection";
 
@@ -49,7 +49,7 @@ export function useEventfulState<T>(accessor: () => T, element: HTMLElement | nu
  * @returns The current value of the accessor.
  */
 // eslint-disable-next-line @typescript-eslint/naming-convention
-export function useObservableState<T>(accessor: () => T, ...observables: Array<Observable<any> | null | undefined>): T {
+export function useObservableState<T>(accessor: () => T, ...observables: Array<IReadonlyObservable | null | undefined>): T {
     const [current, setCurrent] = useState(accessor);
 
     useEffect(() => {
diff --git a/packages/dev/inspector-v2/src/misc/observableCollection.ts b/packages/dev/inspector-v2/src/misc/observableCollection.ts
@@ -1,5 +1,5 @@
 // eslint-disable-next-line import/no-internal-modules
-import type { IDisposable } from "core/index";
+import type { IDisposable, IReadonlyObservable } from "core/index";
 
 import { Observable } from "core/Misc/observable";
 
@@ -9,11 +9,14 @@ import { Observable } from "core/Misc/observable";
 export class ObservableCollection<T> {
     private readonly _items: T[] = [];
     private readonly _keys: symbol[] = [];
+    private readonly _observable = new Observable<void>();
 
     /**
      * An observable that notifies observers when the collection changes.
      */
-    public readonly observable = new Observable<void>();
+    public get observable(): IReadonlyObservable<void> {
+        return this._observable;
+    }
 
     /**
      * The items in the collection.
@@ -31,14 +34,14 @@ export class ObservableCollection<T> {
         const key = Symbol();
         this._items.push(item);
         this._keys.push(key);
-        this.observable.notifyObservers();
+        this._observable.notifyObservers();
 
         return {
             dispose: () => {
                 const index = this._keys.indexOf(key);
                 this._items.splice(index, 1);
                 this._keys.splice(index, 1);
-                this.observable.notifyObservers();
+                this._observable.notifyObservers();
             },
         };
     }
diff --git a/packages/dev/inspector-v2/src/modularity/serviceContainer.ts b/packages/dev/inspector-v2/src/modularity/serviceContainer.ts
@@ -5,13 +5,16 @@ import type { IService, ServiceDefinition } from "./serviceDefinition";
 
 import { SortGraph } from "../misc/graphUtils";
 
+// Service definitions should strongly typed when they are defined to avoid programming errors. However, they are tracked internally
+// in a weakly typed manner so they can be handled generically.
 export type WeaklyTypedServiceDefinition = Omit<ServiceDefinition<IService<symbol>[] | [], IService<symbol>[] | []>, "factory"> & {
     /**
      * A factory function responsible for creating a service instance.
      */
     factory: (...args: any) => ReturnType<ServiceDefinition<IService<symbol>[] | [], IService<symbol>[] | []>["factory"]>;
 };
 
+// This sorts a set of service definitions based on their dependencies (e.g. a topological sort).
 function SortServiceDefinitions(serviceDefinitions: WeaklyTypedServiceDefinition[]) {
     const sortedServiceDefinitions: typeof serviceDefinitions = [];
     SortGraph(
diff --git a/packages/dev/inspector-v2/src/modularity/serviceDefinition.ts b/packages/dev/inspector-v2/src/modularity/serviceDefinition.ts
@@ -22,20 +22,25 @@ export interface IService<ContractIdentity extends symbol> {
     readonly [Contract]?: ContractIdentity;
 }
 
+// This utility type extracts the service identity (unique symbol type) from a service contract. That is, it extracts T from IService<T>.
 type ExtractContractIdentity<ServiceContract extends IService<symbol>> = ServiceContract extends IService<infer ContractIdentity> ? ContractIdentity : never;
 
+// This utility type extracts the contract identities from an array of service contracts. That is, it extracts [T1, T2, ...] from [IService<T1>, IService<T2>, ...].
 type ExtractContractIdentities<ServiceContracts extends IService<symbol>[]> = {
     [Index in keyof ServiceContracts]: ExtractContractIdentity<ServiceContracts[Index]>;
 };
 
+// This utility type converts a union of types into an intersection of types. That is, it converts T1 | T2 | ... into T1 & T2 & ...
+// This is specifically used to determine the type that a service factory function returns when it produces multiple services.
 type UnionToIntersection<Union> = (Union extends any ? (k: Union) => void : never) extends (k: infer Intersection) => void ? Intersection : never;
 
 type MaybePromise<T> = T | Promise<T>;
 
 /**
  * A factory function responsible for creating a service instance.
  * Consumed services are passed as arguments to the factory function.
- * The returned value must implement all produced services.
+ * The returned value must implement all produced services, and may IDisposable.
+ * If not services are produced, the returned value may implement IDisposable, otherwise it may return void.
  */
 export type ServiceFactory<Produces extends IService<symbol>[], Consumes extends IService<symbol>[]> = (
     ...dependencies: [...Consumes, abortSignal?: AbortSignal]
diff --git a/packages/dev/inspector-v2/src/services/panes/scene/nodeExplorerService.tsx b/packages/dev/inspector-v2/src/services/panes/scene/nodeExplorerService.tsx
@@ -1,5 +1,5 @@
 // eslint-disable-next-line import/no-internal-modules
-import type { Observer, Scene } from "core/index";
+import type { IReadonlyObservable, Node, Scene } from "core/index";
 
 import type { ServiceDefinition } from "../../../modularity/serviceDefinition";
 import type { ISceneExplorerService } from "./sceneExplorerService";
@@ -35,55 +35,24 @@ export const NodeHierarchyServiceDefinition: ServiceDefinition<[], [ISceneExplor
                     <></>
                 ),
             watch: (scene, onAdded, onRemoved) => {
-                const observers: Observer<unknown>[] = [];
-
-                observers.push(
-                    scene.onNewMeshAddedObservable.add((mesh) => {
-                        onAdded(mesh);
-                    }) as Observer<unknown>
-                );
-
-                observers.push(
-                    scene.onNewTransformNodeAddedObservable.add((node) => {
-                        onAdded(node);
-                    }) as Observer<unknown>
-                );
-
-                observers.push(
-                    scene.onNewCameraAddedObservable.add((camera) => {
-                        onAdded(camera);
-                    }) as Observer<unknown>
-                );
-
-                observers.push(
-                    scene.onNewLightAddedObservable.add((light) => {
-                        onAdded(light);
-                    }) as Observer<unknown>
-                );
-
-                observers.push(
-                    scene.onMeshRemovedObservable.add((mesh) => {
-                        onRemoved(mesh);
-                    }) as Observer<unknown>
-                );
-
-                observers.push(
-                    scene.onTransformNodeRemovedObservable.add((node) => {
-                        onRemoved(node);
-                    }) as Observer<unknown>
-                );
-
-                observers.push(
-                    scene.onCameraRemovedObservable.add((camera) => {
-                        onRemoved(camera);
-                    }) as Observer<unknown>
-                );
-
-                observers.push(
-                    scene.onLightRemovedObservable.add((light) => {
-                        onRemoved(light);
-                    }) as Observer<unknown>
-                );
+                const observers = [
+                    ...(
+                        [
+                            scene.onNewMeshAddedObservable,
+                            scene.onNewTransformNodeAddedObservable,
+                            scene.onNewCameraAddedObservable,
+                            scene.onNewLightAddedObservable,
+                        ] as IReadonlyObservable<Node>[]
+                    ).map((observable) => observable.add((node) => onAdded(node))),
+                    ...(
+                        [
+                            scene.onMeshRemovedObservable,
+                            scene.onTransformNodeRemovedObservable,
+                            scene.onCameraRemovedObservable,
+                            scene.onLightRemovedObservable,
+                        ] as IReadonlyObservable<Node>[]
+                    ).map((observable) => observable.add((node) => onRemoved(node))),
+                ] as const;
 
                 return {
                     dispose: () => {
diff --git a/packages/dev/inspector-v2/src/services/sceneContext.ts b/packages/dev/inspector-v2/src/services/sceneContext.ts
@@ -1,5 +1,5 @@
 // eslint-disable-next-line import/no-internal-modules
-import type { Nullable, Observable, Scene } from "core/index";
+import type { IReadonlyObservable, Nullable, Scene } from "core/index";
 
 import type { IService } from "../modularity/serviceDefinition";
 
@@ -17,5 +17,5 @@ export interface ISceneContext extends IService<typeof SceneContextIdentity> {
     /**
      * Observable that fires whenever the current scene changes.
      */
-    readonly currentSceneObservable: Observable<Nullable<Scene>>;
+    readonly currentSceneObservable: IReadonlyObservable<Nullable<Scene>>;
 }
diff --git a/packages/dev/inspector-v2/src/services/selectionService.ts b/packages/dev/inspector-v2/src/services/selectionService.ts
@@ -1,5 +1,5 @@
 // eslint-disable-next-line import/no-internal-modules
-import type { Nullable } from "core/index";
+import type { IReadonlyObservable, Nullable } from "core/index";
 
 import type { IService, ServiceDefinition } from "../modularity/serviceDefinition";
 
@@ -19,7 +19,7 @@ export interface ISelectionService extends IService<typeof SelectionServiceIdent
     /**
      * An observable that notifies when the selected entity changes.
      */
-    readonly onSelectedEntityChanged: Observable<void>;
+    readonly onSelectedEntityChanged: IReadonlyObservable<void>;
 }
 
 export const SelectionServiceDefinition: ServiceDefinition<[ISelectionService], []> = {
