merge: adversarial review of the adapter

Author: bebrasmell

src/container/regressions.spec.tsx

@@ -7,14 +7,15 @@ import {
 } from "@illuma/core";
 import { Illuma } from "@illuma/core/plugins";
 import { render } from "@testing-library/react";
-import { Activity } from "react";
+import { Activity, StrictMode } from "react";
 import { afterEach, describe, expect, it, vi } from "vitest";
 import { DiContext } from "./context";
 import { __resetReactDiagnostics, enableReactDiagnostics } from "./diagnostics";
 import { useDiContainer } from "./hooks/container.hook";
 import { useDependency } from "./hooks/dependency.hook";
 import { LIFECYCLE_NODE } from "./lifecycle";
 import { IllumaRoot, ProviderGroup } from "./provider";
+import { ContainerScope, type ScopeOptions } from "./scope";
 import { childHookCount, flush } from "./test-utils";
 
 afterEach(() => {
@@ -317,6 +318,97 @@ describe("the providers-changed warning", () => {
   });
 });
 
+/**
+ * A scope's container is weakly linked to its parent, which is the only reason
+ * it is safe to build one during a render React may throw away. The price of
+ * that link is that a discarded container never runs its destroy hooks, so it
+ * may only ever hold things that are free to drop.
+ */
+describe("eager instantiation", () => {
+  it("never fills a container React might discard", async () => {
+    let runs = 0;
+    const TOKEN = new NodeToken<string>("eager-token");
+
+    const view = render(
+      <StrictMode>
+        <IllumaRoot>
+          <ProviderGroup
+            {...({ instant: true } as ScopeOptions)}
+            providers={[
+              {
+                provide: TOKEN,
+                factory: () => {
+                  runs++;
+                  return "x";
+                },
+              },
+            ]}
+          >
+            <span>leaf</span>
+          </ProviderGroup>
+        </IllumaRoot>
+      </StrictMode>,
+    );
+    await flush();
+
+    // The core still runs a scan pass per container to measure the graph; what
+    // must not happen is a second, real construction nobody will ever destroy.
+    expect(runs).toBeLessThanOrEqual(2);
+
+    view.unmount();
+    await flush();
+    expect(runs).toBeLessThanOrEqual(2);
+  });
+});
+
+/**
+ * A render reads the container, then the deferred release of an earlier unmount
+ * destroys it, and only then does the commit arrive. React yields between the
+ * two whenever the update is a transition, so the microtask that destroys the
+ * container lands squarely in that gap. The rebuild on retain is invisible to
+ * the render that already ran, which is what the listeners are for.
+ */
+describe("a release that lands between a render and its commit", () => {
+  it("tells listeners that the container they read was replaced", async () => {
+    const scope = ContainerScope.create({});
+    const read = scope.getContainer();
+
+    scope.retain();
+    scope.release();
+    await flush();
+    expect(read.destroyed).toBe(true);
+
+    const woken = vi.fn();
+    scope.subscribe(woken);
+
+    scope.retain();
+
+    expect(woken).toHaveBeenCalledTimes(1);
+    expect(scope.getContainer()).not.toBe(read);
+    expect(scope.getContainer().destroyed).toBe(false);
+
+    scope.release();
+    await flush();
+  });
+
+  it("stays quiet when the container it read is still alive", async () => {
+    const scope = ContainerScope.create({});
+    const woken = vi.fn();
+    scope.subscribe(woken);
+
+    scope.retain();
+    scope.release();
+    scope.retain();
+    await flush();
+
+    expect(woken).not.toHaveBeenCalled();
+    expect(scope.getContainer().destroyed).toBe(false);
+
+    scope.release();
+    await flush();
+  });
+});
+
 describe("cross-bundle identity", () => {
   it("keys the context and lifecycle token on globalThis so bundles agree", () => {
     const g = globalThis as Record<symbol, unknown>;

src/container/scope.ts

@@ -7,8 +7,19 @@ import { reactDiagnosticsEnabled, trackProviderUsage } from "./diagnostics";
  * The container options a caller may set. `parent` and `weakParentLink` are
  * withheld because React owns both: the tree decides the parent, and a scope is
  * only safe to build during a render because the link is weak.
+ *
+ * `instant` is withheld because it contradicts that weak link. A weakly linked
+ * container that React discards is never destroyed — nothing observes that it
+ * became unreachable — so it may only ever hold what is free to drop. Eager
+ * instantiation fills a speculative container with live instances whose destroy
+ * hooks will never run. Build the container yourself and hand it to
+ * `<IllumaRoot container={...}>` when eager really is what you want; there its
+ * lifetime is yours and no render can throw it away.
  */
-export type ScopeOptions = Omit<iContainerOptions, "parent" | "weakParentLink">;
+export type ScopeOptions = Omit<
+  iContainerOptions,
+  "parent" | "weakParentLink" | "instant"
+>;
 
 export interface iScopeConfig {
   readonly parent?: NodeContainer;
@@ -155,8 +166,8 @@ export class ContainerScope {
     const { parent, providers, options } = this._config;
 
     const container = new NodeContainer({
-      instant: false,
       ...options,
+      instant: false,
       parent,
       weakParentLink: true,
     });

src/signals/hooks/signal.hook.ts

@@ -1,5 +1,38 @@
 import { isSignal, type ReadonlySignal } from "@illuma/signals";
-import { useSyncExternalStore } from "react";
+import { useRef, useSyncExternalStore } from "react";
+
+interface iSignalStore<T> {
+  readonly signal: ReadonlySignal<T>;
+  readonly subscribe: (onStoreChange: () => void) => () => void;
+  readonly getSnapshot: () => T;
+}
+
+/**
+ * `useSyncExternalStore` requires two consecutive `getSnapshot` calls with no
+ * notification in between to return the very same value, and reading a signal
+ * does not promise that: `external` re-reads its origin on every access while
+ * nothing observes it, so a source that hands back a fresh object each time
+ * yields a different identity per call. React answers that with an infinite
+ * render loop.
+ *
+ * So the value React sees is the last one the signal announced, not whatever a
+ * read would produce right now. The subscription carries the value with it, and
+ * subscribing emits the current one straight away, so the cache cannot start out
+ * behind the signal.
+ */
+function createStore<T>(signalRef: ReadonlySignal<T>): iSignalStore<T> {
+  let snapshot = signalRef();
+
+  return {
+    signal: signalRef,
+    subscribe: (onStoreChange) =>
+      signalRef.subscribe((value) => {
+        snapshot = value;
+        onStoreChange();
+      }),
+    getSnapshot: () => snapshot,
+  };
+}
 
 /**
  * React hook to subscribe to a signal and get its current value.
@@ -12,8 +45,12 @@ export function useSignal<T>(signalRef: ReadonlySignal<T>): T {
     throw new Error("useSignal expects a signal as an argument");
   }
 
+  const store = useRef<iSignalStore<T> | null>(null);
+  if (store.current?.signal !== signalRef) store.current = createStore(signalRef);
+
   // A signal is readable synchronously off the server too, so the server
-  // snapshot is the same read. Without this argument React throws outright
+  // snapshot is the same getter. Without this argument React throws outright
   // during `renderToString`.
-  return useSyncExternalStore(signalRef.subscribe, signalRef, signalRef);
+  const { subscribe, getSnapshot } = store.current;
+  return useSyncExternalStore(subscribe, getSnapshot, getSnapshot);
 }