Revised useSetting and useWebViewState (#728)

lib/papi-dts/papi.d.ts

@@ -180,16 +180,26 @@ declare module 'shared/models/web-view.model' {
    * _＠param_ `stateKey` Key of the state value to use. The webview state holds a unique value per
    * key.
    *
-   * NOTE: `stateKey` needs to be a constant string, not something that could change during execution.
+   * WARNING: MUST BE STABLE - const or wrapped in useState, useMemo, etc. The reference must not be
+   * updated every render
    *
    * _＠param_ `defaultStateValue` Value to use if the web view state didn't contain a value for the
    * given 'stateKey'
    *
-   * _＠returns_ `[stateValue, setStateValue]`
+   * Note: this parameter is internally assigned to a `ref`, so changing it will not cause any hooks
+   * to re-run with its new value. Running `resetWebViewState()` will always update the state value
+   * returned to the latest `defaultStateValue`, and changing the `stateKey` will use the latest
+   * `defaultStateValue`. However, if `defaultStateValue` is changed while a state is
+   * `defaultStateValue` (meaning it is reset and has no value), the returned state value will not be
+   * updated to the new `defaultStateValue`.
+   *
+   * _＠returns_ `[stateValue, setStateValue, resetWebViewState]`
    *
-   * - `stateValue`: the current value for the web view state at the key specified or
+   * - `webViewStateValue`: The current value for the web view state at the key specified or
    *   `defaultStateValue` if a state was not found
-   * - `setStateValue`: function to use to update the web view state value at the key specified
+   * - `setWebViewState`: Function to use to update the web view state value at the key specified
+   * - `resetWebViewState`: Function that removes the web view state and resets the value to
+   *   `defaultStateValue`
    *
    * _＠example_
    *
@@ -200,7 +210,11 @@ declare module 'shared/models/web-view.model' {
   export type UseWebViewStateHook = <T>(
     stateKey: string,
     defaultStateValue: T,
-  ) => [webViewState: T, setWebViewState: (stateValue: T) => void, resetWebViewState: () => void];
+  ) => [
+    webViewStateValue: T,
+    setWebViewState: (stateValue: T) => void,
+    resetWebViewState: () => void,
+  ];
   /**
    *
    * Gets the updatable properties on this WebView's WebView definition
@@ -239,16 +253,26 @@ declare module 'shared/models/web-view.model' {
      * _＠param_ `stateKey` Key of the state value to use. The webview state holds a unique value per
      * key.
      *
-     * NOTE: `stateKey` needs to be a constant string, not something that could change during execution.
+     * WARNING: MUST BE STABLE - const or wrapped in useState, useMemo, etc. The reference must not be
+     * updated every render
      *
      * _＠param_ `defaultStateValue` Value to use if the web view state didn't contain a value for the
      * given 'stateKey'
      *
-     * _＠returns_ `[stateValue, setStateValue]`
+     * Note: this parameter is internally assigned to a `ref`, so changing it will not cause any hooks
+     * to re-run with its new value. Running `resetWebViewState()` will always update the state value
+     * returned to the latest `defaultStateValue`, and changing the `stateKey` will use the latest
+     * `defaultStateValue`. However, if `defaultStateValue` is changed while a state is
+     * `defaultStateValue` (meaning it is reset and has no value), the returned state value will not be
+     * updated to the new `defaultStateValue`.
+     *
+     * _＠returns_ `[stateValue, setStateValue, resetWebViewState]`
      *
-     * - `stateValue`: the current value for the web view state at the key specified or
+     * - `webViewStateValue`: The current value for the web view state at the key specified or
      *   `defaultStateValue` if a state was not found
-     * - `setStateValue`: function to use to update the web view state value at the key specified
+     * - `setWebViewState`: Function to use to update the web view state value at the key specified
+     * - `resetWebViewState`: Function that removes the web view state and resets the value to
+     *   `defaultStateValue`
      *
      * _＠example_
      *
@@ -349,16 +373,26 @@ declare module 'shared/global-this.model' {
      * _＠param_ `stateKey` Key of the state value to use. The webview state holds a unique value per
      * key.
      *
-     * NOTE: `stateKey` needs to be a constant string, not something that could change during execution.
+     * WARNING: MUST BE STABLE - const or wrapped in useState, useMemo, etc. The reference must not be
+     * updated every render
      *
      * _＠param_ `defaultStateValue` Value to use if the web view state didn't contain a value for the
      * given 'stateKey'
      *
-     * _＠returns_ `[stateValue, setStateValue]`
+     * Note: this parameter is internally assigned to a `ref`, so changing it will not cause any hooks
+     * to re-run with its new value. Running `resetWebViewState()` will always update the state value
+     * returned to the latest `defaultStateValue`, and changing the `stateKey` will use the latest
+     * `defaultStateValue`. However, if `defaultStateValue` is changed while a state is
+     * `defaultStateValue` (meaning it is reset and has no value), the returned state value will not be
+     * updated to the new `defaultStateValue`.
      *
-     * - `stateValue`: the current value for the web view state at the key specified or
+     * _＠returns_ `[stateValue, setStateValue, resetWebViewState]`
+     *
+     * - `webViewStateValue`: The current value for the web view state at the key specified or
      *   `defaultStateValue` if a state was not found
-     * - `setStateValue`: function to use to update the web view state value at the key specified
+     * - `setWebViewState`: Function to use to update the web view state value at the key specified
+     * - `resetWebViewState`: Function that removes the web view state and resets the value to
+     *   `defaultStateValue`
      *
      * _＠example_
      *
@@ -4048,22 +4082,30 @@ declare module 'renderer/hooks/papi-hooks/use-setting.hook' {
    * @param defaultState The default state of the setting. If the setting already has a value set to
    *   it in local storage, this parameter will be ignored.
    *
-   *   WARNING: MUST BE STABLE - const or wrapped in useState, useMemo, etc. The reference must not be
-   *   updated every render
+   *   Note: this parameter is internally assigned to a `ref`, so changing it will not cause any hooks
+   *   to re-run with its new value. Running `resetSetting()` will always update the setting value
+   *   returned to the latest `defaultState`, and changing the `key` will use the latest
+   *   `defaultState`. However, if `defaultState` is changed while a setting is `defaultState`
+   *   (meaning it is reset and has no value), the returned setting value will not be updated to the
+   *   new `defaultState`.
    * @returns `[setting, setSetting, resetSetting]`
    *
-   *   - `setting`: The current state of the setting, either the defaultState or the stored state on the
+   *   - `setting`: The current state of the setting, either `defaultState` or the stored state on the
    *       papi, if any
    *   - `setSetting`: Function that updates the setting to a new value
-   *   - `resetSetting`: Function that removes the setting
+   *   - `resetSetting`: Function that removes the setting and resets the value to `defaultState`
    *
    * @throws When subscription callback function is called with an update that has an unexpected
    *   message type
    */
   const useSetting: <SettingName extends keyof SettingTypes>(
     key: SettingName,
     defaultState: SettingTypes[SettingName],
-  ) => [SettingTypes[SettingName], (newSetting: SettingTypes[SettingName]) => void, () => void];
+  ) => [
+    setting: SettingTypes[SettingName],
+    setSetting: (newSetting: SettingTypes[SettingName]) => void,
+    resetSetting: () => void,
+  ];
   export default useSetting;
 }
 declare module 'renderer/hooks/papi-hooks/use-project-data-provider.hook' {

src/renderer/hooks/papi-hooks/use-setting.hook.ts

@@ -1,4 +1,4 @@
-import { useCallback, useEffect, useState } from 'react';
+import { useCallback, useEffect, useRef, useState } from 'react';
 import settingsService from '@shared/services/settings.service';
 import { SettingNames, SettingTypes } from 'papi-shared-types';
 
@@ -13,37 +13,46 @@ import { SettingNames, SettingTypes } from 'papi-shared-types';
  * @param defaultState The default state of the setting. If the setting already has a value set to
  *   it in local storage, this parameter will be ignored.
  *
- *   WARNING: MUST BE STABLE - const or wrapped in useState, useMemo, etc. The reference must not be
- *   updated every render
+ *   Note: this parameter is internally assigned to a `ref`, so changing it will not cause any hooks
+ *   to re-run with its new value. Running `resetSetting()` will always update the setting value
+ *   returned to the latest `defaultState`, and changing the `key` will use the latest
+ *   `defaultState`. However, if `defaultState` is changed while a setting is `defaultState`
+ *   (meaning it is reset and has no value), the returned setting value will not be updated to the
+ *   new `defaultState`.
  * @returns `[setting, setSetting, resetSetting]`
  *
- *   - `setting`: The current state of the setting, either the defaultState or the stored state on the
+ *   - `setting`: The current state of the setting, either `defaultState` or the stored state on the
  *       papi, if any
  *   - `setSetting`: Function that updates the setting to a new value
- *   - `resetSetting`: Function that removes the setting
+ *   - `resetSetting`: Function that removes the setting and resets the value to `defaultState`
  *
  * @throws When subscription callback function is called with an update that has an unexpected
  *   message type
  */
 const useSetting = <SettingName extends SettingNames>(
   key: SettingName,
   defaultState: SettingTypes[SettingName],
-): [SettingTypes[SettingName], (newSetting: SettingTypes[SettingName]) => void, () => void] => {
-  const [setting, setSettingInternal] = useState(settingsService.get(key, defaultState));
+): [
+  setting: SettingTypes[SettingName],
+  setSetting: (newSetting: SettingTypes[SettingName]) => void,
+  resetSetting: () => void,
+] => {
+  // Use defaultState as a ref so it doesn't update dependency arrays
+  const defaultStateRef = useRef(defaultState);
+  defaultStateRef.current = defaultState;
 
-  useEffect(() => {
-    const updateSettingFromService = (newSettingState: SettingTypes[SettingName]) => {
-      setSettingInternal(newSettingState);
-    };
+  const [setting, setSettingInternal] = useState(settingsService.get(key, defaultStateRef.current));
 
-    const initialSetting = settingsService.get(key, defaultState);
-    updateSettingFromService(initialSetting);
+  useEffect(() => {
+    // Get the setting for the new key when the key changes
+    setSettingInternal(settingsService.get(key, defaultStateRef.current));
 
+    // Subscribe to changes to the setting
     const unsubscriber = settingsService.subscribe(key, (newSetting) => {
       if (newSetting.type === 'update-setting') {
-        updateSettingFromService(newSetting.setting);
+        setSettingInternal(newSetting.setting);
       } else if (newSetting.type === 'reset-setting') {
-        setSettingInternal(defaultState);
+        setSettingInternal(defaultStateRef.current);
       } else {
         throw new Error('Unexpected message type used for updating setting');
       }
@@ -52,7 +61,7 @@ const useSetting = <SettingName extends SettingNames>(
     return () => {
       unsubscriber();
     };
-  }, [defaultState, key]);
+  }, [key]);
 
   const setSetting = useCallback(
     (newSetting: SettingTypes[SettingName]) => {
@@ -64,8 +73,8 @@ const useSetting = <SettingName extends SettingNames>(
 
   const resetSetting = useCallback(() => {
     settingsService.reset(key);
-    setSettingInternal(defaultState);
-  }, [defaultState, key]);
+    setSettingInternal(defaultStateRef.current);
+  }, [key]);
 
   return [setting, setSetting, resetSetting];
 };

src/renderer/hooks/use-web-view-state.hook.ts

@@ -1,4 +1,4 @@
-import { useState, useCallback, useEffect } from 'react';
+import { useState, useCallback, useEffect, useRef } from 'react';
 
 // We don't add this to PAPI directly like other hooks because `this` has to be bound to a web view's iframe context
 /** See `web-view.model.ts` for normal hook documentation */
@@ -10,31 +10,36 @@ export default function useWebViewState<T>(
   },
   stateKey: string,
   defaultStateValue: T,
-): [webViewState: T, setWebViewState: (newStateValue: T) => void, resetWebViewState: () => void] {
+): [
+  webViewStateValue: T,
+  setWebViewState: (newStateValue: T) => void,
+  resetWebViewState: () => void,
+] {
+  // Use defaultStateValue as a ref so it doesn't update dependency arrays
+  const defaultStateValueRef = useRef(defaultStateValue);
+  defaultStateValueRef.current = defaultStateValue;
+
   const [state, setStateInternal] = useState(() =>
-    this.getWebViewState(stateKey, defaultStateValue),
+    this.getWebViewState(stateKey, defaultStateValueRef.current),
   );
 
   useEffect(() => {
-    if (
-      this.getWebViewState(stateKey, defaultStateValue) === defaultStateValue &&
-      state !== defaultStateValue
-    )
-      setStateInternal(defaultStateValue);
-  }, [defaultStateValue, state, stateKey]);
+    // Get the setting for the new key when the key changes
+    setStateInternal(this.getWebViewState(stateKey, defaultStateValueRef.current));
+  }, [stateKey]);
 
   const setState = useCallback(
     (newStateValue: T) => {
-      setStateInternal(newStateValue);
       this.setWebViewState(stateKey, newStateValue);
+      setStateInternal(newStateValue);
     },
     [stateKey],
   );
 
   const resetState = useCallback(() => {
-    setStateInternal(defaultStateValue);
     this.resetWebViewState(stateKey);
-  }, [defaultStateValue, stateKey]);
+    setStateInternal(defaultStateValueRef.current);
+  }, [stateKey]);
 
   return [state, setState, resetState];
 }

src/shared/models/web-view.model.ts

@@ -199,16 +199,26 @@ export type WebViewDefinitionUpdateInfo = Partial<WebViewDefinitionUpdatableProp
  * _＠param_ `stateKey` Key of the state value to use. The webview state holds a unique value per
  * key.
  *
- * NOTE: `stateKey` needs to be a constant string, not something that could change during execution.
+ * WARNING: MUST BE STABLE - const or wrapped in useState, useMemo, etc. The reference must not be
+ * updated every render
  *
  * _＠param_ `defaultStateValue` Value to use if the web view state didn't contain a value for the
  * given 'stateKey'
  *
- * _＠returns_ `[stateValue, setStateValue]`
+ * Note: this parameter is internally assigned to a `ref`, so changing it will not cause any hooks
+ * to re-run with its new value. Running `resetWebViewState()` will always update the state value
+ * returned to the latest `defaultStateValue`, and changing the `stateKey` will use the latest
+ * `defaultStateValue`. However, if `defaultStateValue` is changed while a state is
+ * `defaultStateValue` (meaning it is reset and has no value), the returned state value will not be
+ * updated to the new `defaultStateValue`.
  *
- * - `stateValue`: the current value for the web view state at the key specified or
+ * _＠returns_ `[stateValue, setStateValue, resetWebViewState]`
+ *
+ * - `webViewStateValue`: The current value for the web view state at the key specified or
  *   `defaultStateValue` if a state was not found
- * - `setStateValue`: function to use to update the web view state value at the key specified
+ * - `setWebViewState`: Function to use to update the web view state value at the key specified
+ * - `resetWebViewState`: Function that removes the web view state and resets the value to
+ *   `defaultStateValue`
  *
  * _＠example_
  *
@@ -219,7 +229,11 @@ export type WebViewDefinitionUpdateInfo = Partial<WebViewDefinitionUpdatableProp
 export type UseWebViewStateHook = <T>(
   stateKey: string,
   defaultStateValue: T,
-) => [webViewState: T, setWebViewState: (stateValue: T) => void, resetWebViewState: () => void];
+) => [
+  webViewStateValue: T,
+  setWebViewState: (stateValue: T) => void,
+  resetWebViewState: () => void,
+];
 
 // Note: the following comment uses ＠, not the actual @ character, to hackily provide @param and
 // such on this type. It seem that, for some reason, JSDoc does not carry these annotations on