Make subscriptions re-usable. Fixes strict mode.

saasquatch · Dec 13, 2023 · 326c4cf · 326c4cf
1 parent daa0b0e
commit 326c4cf
Show file tree

Hide file tree

Showing 11 changed files with 324 additions and 207 deletions.
diff --git a/src/react/ComponentScope.test.tsx b/src/react/ComponentScope.test.tsx
@@ -2,7 +2,6 @@ import { act, renderHook } from "@testing-library/react";
 import { atom, useAtom } from "jotai";
 import { createLifecycleUtils } from "../shared/testing/lifecycle";
 import { ComponentScope, molecule, resetDefaultInjector, use } from "./";
-import { createTestInjectorProvider } from "./testing/TestInjectorProvider";
 import { strictModeSuite } from "./testing/strictModeSuite";
 import { useMolecule } from "./useMolecule";
 
@@ -30,16 +29,20 @@ const useCounter = (mol: typeof ComponentScopedCountMolecule) => {
 };
 
 beforeEach(() => {
-  resetDefaultInjector();
+  resetDefaultInjector({
+    //  instrumentation: new LoggingInstrumentation()
+  });
+  compLifecycle.reset();
 });
+
 strictModeSuite(({ wrapper: Outer }) => {
   describe("Global scoped molecules", () => {
-    test("should increment counter", () => {
+    test("one counter", () => {
       globalCompLifecycle.expectUncalled();
       testOneCounter(GlobalScopedMoleculeCountMolecule, 1);
-      expect.soft(globalCompLifecycle.executions).toHaveBeenCalledOnce();
+      expect(globalCompLifecycle.executions).toHaveBeenCalledOnce();
       expect.soft(globalCompLifecycle.mounts).toHaveBeenCalledOnce();
-      expect.soft(globalCompLifecycle.unmounts).toHaveBeenCalledOnce();
+      expect(globalCompLifecycle.unmounts).toHaveBeenCalledOnce();
     });
     test("two counters should be connected for global scope", () => {
       // Note: This is an important test case, because
@@ -62,7 +65,7 @@ strictModeSuite(({ wrapper: Outer }) => {
   });
 
   describe("Component scoped molecules", () => {
-    test("should increment counter", () => {
+    test("one counter", () => {
       compLifecycle.expectUncalled();
       testOneCounter(ComponentScopedCountMolecule, 1);
 
@@ -83,12 +86,16 @@ strictModeSuite(({ wrapper: Outer }) => {
       expect(compLifecycle.unmounts).toHaveBeenCalledTimes(2);
     });
     test("two counters should be not be connected when component scoped, use first one", () => {
+      compLifecycle.expectUncalled();
       testTwoCounters(ComponentScopedCountMolecule, {
         actual1: true,
         actual2: false,
         expected1: 1,
         expected2: 0,
       });
+      expect(compLifecycle.executions).toHaveBeenCalledTimes(2);
+      expect(compLifecycle.mounts).toHaveBeenCalledTimes(2);
+      expect(compLifecycle.unmounts).toHaveBeenCalledTimes(2);
     });
     test("two counters should be not be connected when component scoped, use second one", () => {
       testTwoCounters(ComponentScopedCountMolecule, {
@@ -97,18 +104,17 @@ strictModeSuite(({ wrapper: Outer }) => {
         expected1: 0,
         expected2: 1,
       });
+      expect(compLifecycle.executions).toHaveBeenCalledTimes(2);
+      expect(compLifecycle.mounts).toHaveBeenCalledTimes(2);
+      expect(compLifecycle.unmounts).toHaveBeenCalledTimes(2);
     });
   });
 
   function testOneCounter(
     mol: typeof ComponentScopedCountMolecule,
     expectedResult: number,
   ) {
-    const TestInjectorProvider = createTestInjectorProvider(Outer);
-
-    const { result, ...rest } = renderHook(() => useCounter(mol), {
-      wrapper: TestInjectorProvider,
-    });
+    const { result, ...rest } = renderHook(() => useCounter(mol));
 
     act(() => {
       result.current.increment();
@@ -128,14 +134,8 @@ strictModeSuite(({ wrapper: Outer }) => {
       expected1: number;
     },
   ) {
-    const TestInjectorProvider = createTestInjectorProvider(Outer);
-
-    const { result: result1, ...rest1 } = renderHook(() => useCounter(mol), {
-      wrapper: TestInjectorProvider,
-    });
-    const { result: result2, ...rest2 } = renderHook(() => useCounter(mol), {
-      wrapper: TestInjectorProvider,
-    });
+    const { result: result1, ...rest1 } = renderHook(() => useCounter(mol));
+    const { result: result2, ...rest2 } = renderHook(() => useCounter(mol));
 
     act(() => {
       opts.actual1 && result1.current.increment();

diff --git a/src/react/ScopeProvider.tsx b/src/react/ScopeProvider.tsx
@@ -1,4 +1,4 @@
-import React from "react";
+import React, { useMemo } from "react";
 import { MoleculeScopeOptions } from "../shared/MoleculeScopeOptions";
 import { ComponentScope, MoleculeScope } from "../vanilla";
 import { ScopeContext } from "./contexts/ScopeContext";
@@ -39,9 +39,13 @@ export function ScopeProvider<T>(
     };
   }
 
-  // FIXME: This sends a new array downstream to the ScopeContext on every render
-  const downstreamScopes = useScopes(options).filter(
-    ([scope]) => scope !== ComponentScope,
+  const simpleDownstreamScopes = useScopes(options);
+
+  // This prevents a new array from being passed downstream on every render
+  // in theory this should reduce context re-renders
+  const downstreamScopes = useMemo(
+    () => simpleDownstreamScopes.filter(([scope]) => scope !== ComponentScope),
+    [simpleDownstreamScopes],
   );
 
   return React.createElement(

diff --git a/src/react/useMolecule.test.tsx b/src/react/useMolecule.test.tsx
@@ -1,19 +1,11 @@
 import { renderHook } from "@testing-library/react";
 import React, { StrictMode, useContext } from "react";
-import {
-  createScope,
-  molecule,
-  onMount,
-  onUnmount,
-  resetDefaultInjector,
-  use,
-} from ".";
+import { createScope, molecule, onMount, onUnmount, use } from ".";
+import { createLifecycleUtils } from "../shared/testing/lifecycle";
 import { ScopeProvider } from "./ScopeProvider";
 import { ScopeContext } from "./contexts/ScopeContext";
 import { strictModeSuite } from "./testing/strictModeSuite";
 import { useMolecule } from "./useMolecule";
-import { createLifecycleUtils } from "../shared/testing/lifecycle";
-import { LoggingInstrumentation } from "../vanilla/internal/instrumentation";
 
 export const UserScope = createScope("[email protected]", {
   debugLabel: "User Scope",
@@ -220,8 +212,7 @@ strictModeSuite(({ wrapper }) => {
   });
 });
 
-test.only("Strict mode", () => {
-  resetDefaultInjector({ instrumentation: new LoggingInstrumentation() });
+test("Strict mode", () => {
   const expectedUser = "[email protected]";
 
   const lifecycle = createLifecycleUtils();
@@ -240,44 +231,47 @@ test.only("Strict mode", () => {
 
   lifecycle.expectUncalled();
 
-  console.log("Render 1");
   const run1 = renderHook(testHook, {
     wrapper: StrictMode,
   });
-  console.log("==========Render 1 done");
 
-  expect(lifecycle.executions).toBeCalledTimes(2);
-  expect(lifecycle.mounts).toBeCalledTimes(1);
-  expect(lifecycle.unmounts).toBeCalledTimes(1);
+  function expectActiveMounted() {
+    // Then execution are called twice
+    // Because once for each render in strict mode
+    expect(lifecycle.executions).toBeCalledTimes(2);
+    // Then mounts are called twice
+    // Because of useEffects called twice in strict mode
+    expect(lifecycle.mounts).toBeCalledTimes(2);
+    // The unount is called once
+    // To cleanup from the original useEffect
+    expect(lifecycle.unmounts).toBeCalledTimes(1);
+  }
+  expectActiveMounted();
   expect(run1.result.current.molecule).toBe(expectedUser);
 
-  console.log("==========Render 2");
   const run2 = renderHook(testHook, {
     wrapper: StrictMode,
   });
-  console.log("Render 2 done");
 
-  expect(lifecycle.executions).toBeCalledTimes(3);
-  expect(lifecycle.mounts).toBeCalledTimes(2);
-  expect(lifecycle.unmounts).toBeCalledTimes(1);
+  /**
+   * Then nothing has changed in the molecule lifecycle
+   * Because a subscription was active at render time
+   * And it re-uses the cached value
+   */
+  expectActiveMounted();
   expect(run2.result.current.molecule).toBe(expectedUser);
 
-  console.log("Unmount 1");
   run1.unmount();
-  console.log("Unmount 1 done");
 
-  expect(lifecycle.executions).toBeCalledWith(expectedUser);
-  expect(lifecycle.mounts).toBeCalledWith(expectedUser);
-  expect(lifecycle.executions).toBeCalledTimes(3);
-  expect(lifecycle.mounts).toBeCalledTimes(2);
-  expect(lifecycle.unmounts).toBeCalledTimes(1);
-  expect(run1.result.current.molecule).toBe(expectedUser);
+  /**
+   * Then nothing has changed in the molecule lifecycle
+   * Because a subscription is still active
+   */
+  expectActiveMounted();
 
-  console.log("Unmount 2");
   run2.unmount();
-  console.log("Unmount 2 done");
 
-  expect(lifecycle.executions).toBeCalledTimes(3);
+  expect(lifecycle.executions).toBeCalledTimes(2);
   expect(lifecycle.mounts).toBeCalledTimes(2);
   // Unmounts are called
   expect(lifecycle.unmounts).toBeCalledTimes(2);

diff --git a/src/react/useMolecule.tsx b/src/react/useMolecule.tsx
@@ -1,4 +1,4 @@
-import { useEffect, useMemo } from "react";
+import { useEffect, useMemo, useState } from "react";
 import { MoleculeScopeOptions } from "../shared/MoleculeScopeOptions";
 import { MoleculeOrInterface } from "../vanilla";
 import type { ScopeProvider } from "./ScopeProvider";
@@ -27,19 +27,36 @@ export function useMolecule<T>(
 ): T {
   const injector = useInjector();
 
-  // FIXME: Memoize these so a new handle is only created when the tuples change
   const inputTuples = useScopeTuplesRaw(options);
-  const [value, handle] = useMemo(
-    () => injector.useLazily(mol, ...inputTuples),
-    [mol, injector, flattenTuples(inputTuples)],
-  );
+  const [value, handle] = useMemo(() => {
+    // console.log("==== fresh Memo! =====");
+
+    return injector.useLazily(mol, ...inputTuples);
+  }, [
+    mol,
+    injector,
+    /**
+     * Tuple flattening prevents re-renders unless the number of
+     */
+    /**
+     *  FIXME: Write some tests that confirm that input tuples can be reactive, but not TOO reative.
+     *
+     *  This was commented out because it made the ComponentScope test fail and cause too many memoized renders
+     */
+    // flattenTuples(inputTuples),
+  ]);
+
+  const [multableValue, setMutableValue] = useState(value);
 
   useEffect(() => {
-    handle.start();
+    // console.log("==== useEffect! =====");
+    const subbedValue = handle.start();
+    setMutableValue(subbedValue);
     return () => {
+      // console.log("==== CLEANUP useEffect! =====");
       handle.stop();
     };
   }, [handle]);
 
-  return value;
+  return multableValue;
 }
diff --git a/src/react/useScopes.tsx b/src/react/useScopes.tsx
@@ -61,6 +61,7 @@ export function useScopeTuplesRaw(options?: MoleculeScopeOptions) {
   const componentScopeTuple = useRef([ComponentScope, generatedValue] as const)
     .current as ScopeTuple<unknown>;
 
+  // FIXME: Memoize these so a new handle is only created when the tuples change
   const inputTuples: AnyScopeTuple[] = (() => {
     if (!options) return [...parentScopes, componentScopeTuple];
     if (options.withUniqueScope) {