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.

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

Revise hooks - adjust which arguments should be reactive #552

Closed
12 tasks done
tjcouch-sil opened this issue Oct 13, 2023 · 2 comments · Fixed by #728
Closed
12 tasks done

Revise hooks - adjust which arguments should be reactive #552

tjcouch-sil opened this issue Oct 13, 2023 · 2 comments · Fixed by #728
Assignees
Labels
enhancement New feature or request

Comments

@tjcouch-sil
Copy link
Member

tjcouch-sil commented Oct 13, 2023

User Story
As an extension developer, I want the React hooks exposed on the papi to make sense and have parameters with appropriate reactivity to existing React hooks and consensus so that I can use hooks effectively without bugs or unnecessary rerenders.

Description
Update our hooks in the following ways:

  • Make key/selector changes retrieve the value again instead of overwriting the existing value in any context like settings, web view state, data, etc. For example, use-setting.hook.ts and use-data.hook.ts currently retrieve the new value when the key/selector is changed
    • Rename use-webview-state.hook.ts to use-web-view-state.hook.ts
    • Make use-web-view-state.hook.ts get the new value at the key when the key is changed instead of overwriting the value at the new key when the key is changed. Currently, web view state keeps the current value and overwrites the value at the new key.
  • Make defaultValue and options parameters refs (useRef) so they are not included in dependency arrays (matches defaultValue better with how it works in useState). Therefore, they will not cause likely unintended recalculations of dependency-based hooks like useEffect and useCallback. Using refs for these does, however, allow callbacks and other things that can re-initiate a need for a defaultValue to honor the most current value of these parameters. In some contexts, this is simply to be expected from these parameters. However, in other cases, it is not naturally expected but is still a benefit. As such, we should document accordingly: the JSDoc for each of these parameters should have a new paragraph with "Note: this parameter is internally assigned to a ref, so changing it will not cause any hooks to re-run with its new value." along with the implications of using a ref for this parameter specifically in each context. Indicate very clearly in the JSDoc that these parameters are NOT listened to in dependency arrays; therefore another argument must be changed or the component must be remounted in order for the defaultValue to be updated.
    • Make defaultState not reactive in use-setting.hook.ts
      • Explanation of the situation with useSetting for practice with the others: In the current implementation of useSetting, defaultValue is a dependency on a useEffect and a useCallback. When defaultValue is changed to be a ref, changing defaultValue will no longer cause the useEffect to re-fetch the setting and cause the returned values to change (which could cause more recalculations or rerenders depending on how a component using this hook uses it). This means changing defaultValue will no longer update the returned state to the new defaultValue if it is currently on the previous defaultValue. However, since it is a ref, running setSetting(undefined) will still set the setting to the newest defaultValue.
        • The JSDoc note for this could look like this: "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 setSetting(undefined) will always use the latest defaultValue. However, if defaultValue is changed while a setting is undefined, the returned setting value will not be updated to the new defaultValue."
    • Make defaultValue not reactive in use-promise.hook.ts
    • subscriberOptions in create-use-data-hook.util.ts (and make sure to update the JSDocs for useData and useProjectData)
    • Change use-promise.hook.ts's preserveValue boolean to an options object with preserveValue inside of it, and make that options object a ref
      • Note that this has no impact on the naturally understood functionality of preserveValue, so something like "Note: this parameter is internally assigned to a ref, so changing it will not cause any hooks to re-run with its new value. However, its latest value will be used appropriately." works for this one
  • Rework useDialogCallback to accept a response callback and a reject callback instead of having bunches of possibly unnecessary states/rerenders and complicated api
    • parameters: dialogType, options, resolveCallback, rejectCallback?
      • dialogType is a ref - please include note in JSDoc as mentioned above
      • options is a ref - please include note in JSDoc as mentioned above
      • resolveCallback runs with the promise's resolved value. Reactive - run the latest when the dialog call resolves
        • parameters: response, dialogType, options (pass in the dialogType and options that were sent to it)
      • rejectCallback runs with the promise's rejected value if an error occurs (catch). Reactive - run the latest when the dialog call rejects
        • parameters: errorMessage, dialogType, options (pass in the dialogType and options that were sent to it)
        • this is optional. If not provided, just log an error if the dialog call rejects
      • Please add a note in the JSDocs on all these parameters that they are refs. See above for more info on these
    • returns function showDialog - showDialog() - function to run to show the dialog
      • Returns: promise that resolves when the dialog resolves? If this is relatively straight-forward
  • Create a README.md in the hooks directory that explains these guiding principles around making hooks
@tjcouch-sil tjcouch-sil added the enhancement New feature or request label Oct 13, 2023
@tjcouch-sil tjcouch-sil moved this to Open in Paranext Oct 13, 2023
@tjcouch-sil tjcouch-sil moved this from Open to Idea Bucket in Paranext Oct 23, 2023
@katherinejensen00 katherinejensen00 moved this from Idea Bucket to On Deck in Paranext Nov 8, 2023
@katherinejensen00 katherinejensen00 moved this from On Deck to Product Backlog in Paranext Nov 13, 2023
@tjcouch-sil tjcouch-sil added this to the 2023-12 Q4 Maintenance milestone Nov 14, 2023
@tjcouch-sil tjcouch-sil added the needs design Waiting on further design decisions and clarification label Nov 14, 2023
@tjcouch-sil tjcouch-sil removed the needs design Waiting on further design decisions and clarification label Dec 11, 2023
@katherinejensen00 katherinejensen00 moved this from 🎬 Product Backlog to 📥 On Deck in Paranext Dec 13, 2023
@katherinejensen00 katherinejensen00 moved this from 📥 On Deck to 🔖 ToDo in Paranext Dec 13, 2023
@tjcouch-sil tjcouch-sil changed the title Revise hooks - consider which arguments should be reactive Revise hooks - djust which arguments should be reactive Dec 14, 2023
@tjcouch-sil tjcouch-sil changed the title Revise hooks - djust which arguments should be reactive Revise hooks - adjust which arguments should be reactive Dec 14, 2023
@tjcouch-sil
Copy link
Member Author

Here is an example diff showing how to make parameters act as refs:

Image

@tjcouch-sil tjcouch-sil moved this from 🔖 ToDo to 🏗 In progress in Paranext Jan 3, 2024
@tjcouch-sil tjcouch-sil self-assigned this Jan 3, 2024
@tjcouch-sil tjcouch-sil moved this from 🏗 In progress to 👀 In review in Paranext Jan 10, 2024
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Paranext Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

1 participant