From a07cc9751d380594882eabb8d4a0734d570df00f Mon Sep 17 00:00:00 2001 From: David Groomes Date: Sun, 7 Jan 2024 15:48:14 -0600 Subject: [PATCH] Replace useFetch with plain useEffect statements. I have struggled to figure out how to compose custom hooks that have effects. --- README.md | 5 ++ src/store.ts | 16 +++--- src/useFetch.ts | 149 ------------------------------------------------ src/useToken.ts | 141 +++++++++++++++++++-------------------------- 4 files changed, 72 insertions(+), 239 deletions(-) delete mode 100644 src/useFetch.ts diff --git a/README.md b/README.md index 5fe13cc..ec8de74 100644 --- a/README.md +++ b/README.md @@ -143,6 +143,10 @@ General clean-ups, todos and things I wish to implement for this project: and not using semicolons. * [x] DONE (fixed, but `useFetch` now doesn't make sense. How do people do this? Just ignore unmounts for the clean up function?) Defect: validation fetch request is getting cancelled prematurely. My `useFetch` must be buggy. +* [x] DONE Stop setting state from render function. During the redux conversion, I started getting warnings about setting + state from the render function. Totally (well 80%) makes sense to me, so I'll fix it. This is part of the process of + grokking React. The trouble is in `useToken`. At this time, it's time to drop `useFetch` which I had previously marked + as deprecated. It's so hard to make this work. ## Finished Wish List Items @@ -171,6 +175,7 @@ General clean-ups, todos and things I wish to implement for this project: * [x] DONE Upgrade dependencies. * [x] DONE (GraphiQL defines fonts in data URLs) Why are there HTTP request failures to download fonts? E.g. `data:font/woff2;base64,` etc. This happens when serving but not in the production app. +* [ ] More robust error handling. We at least want to model basic error states and propage a basic message. ## Reference diff --git a/src/store.ts b/src/store.ts index 0d6074e..0d3597c 100644 --- a/src/store.ts +++ b/src/store.ts @@ -1,13 +1,15 @@ -import {configureStore} from "@reduxjs/toolkit"; +import { configureStore } from "@reduxjs/toolkit"; import monolithicReducer from "./monolithicSlice"; // @ts-ignore -import {customizeWebpackConfigForDevelopment} from "redux-config-customizer"; +import { customizeWebpackConfigForDevelopment } from "redux-config-customizer"; -export const store = configureStore(customizeWebpackConfigForDevelopment({ - reducer: { - monolithic: monolithicReducer, - } -})); +export const store = configureStore( + customizeWebpackConfigForDevelopment({ + reducer: { + monolithic: monolithicReducer, + }, + }), +); // I don't understand how this works, but this is needed when using Redux Toolkit with TypeScript. // See https://redux.js.org/tutorials/typescript-quick-start diff --git a/src/useFetch.ts b/src/useFetch.ts deleted file mode 100644 index d39c3fe..0000000 --- a/src/useFetch.ts +++ /dev/null @@ -1,149 +0,0 @@ -/* -DEPRECATED: DO NOT USE THIS. See the later comment about aborting fetch requests upon unmounting and why that's a -problem. - -Most React applications make HTTP requests, and they do that within a 'useEffect' hook (well, do we really really really need to do that??). -There is a fair amount of boilerplate that goes along with this. Namely, we want to keep track of whether the component -is mounted or not, and we want to abort the request if the component is unmounted. - -The 'useFetch' hook defined here is a 'fetch' wrapper for React. The 'useFetch' hook takes the same parameters as 'fetch' -but instead of returning a promise, it returns the state of the 'fetch' operation: 'in-flight', 'error', or the response -data. It is vaguely similar to the main function signature of *TanStack Query* (formerly known as *React Query*). - -Consider using 'useFetch' under the following conditions: - -* You want to do exactly one HTTP request when the component is mounted -* You want to not update state if the fetch completes after the component is mounted (avoid memory leaks) -* You want the result of the fetch cached. Subsequent invocations of 'useEffect', given the same parameters, return the - cached data. -* You are targeting browsers that support the 'fetch' API -* You are calling an HTTP endpoint that returns JSON in the response body. - -This snippet is a helper function that takes care of all of that boilerplate for you. Namely, it uses the 'fetch' API -and the abort types (AbortController, or AbortSignal). Here's an example: - -function MyDashboardComponent(props) { - const options = { - method: "POST", - headers: { - "Content-Type": "application/json" - }, - body: JSON.stringify({ query: props.query }), - } - const dashboardData = useFetch("https://api.github.com/graphql", options) - - if (dashboardData === 'in-flight') { - return - } - - if (dashboardData === 'error') { - return - } - - // Build the dashboard UI from the data - ... omitted ... -} - */ - -import { useEffect } from "react"; -import { logger } from "./code"; -import { useAppDispatch, useAppSelector } from "./hooks"; -import { FetchState, setFetchState } from "./monolithicSlice"; - -const log = logger("useFetch"); - -export function useFetch(fetchParams: { input: RequestInfo | URL; init?: RequestInit } | "no-op"): FetchState { - if (fetchParams === "no-op") { - log("Invoked as a no-op."); - } else { - log("Invoked with fetch parameters."); - } - const dispatch = useAppDispatch(); - let state = useAppSelector((state) => state.monolithic.fetchState); - - useEffect(() => { - if (fetchParams === "no-op") { - // This is so odd. The React way is to execute "render" functions as the main engine of the program and so - // we as application developers have to deal with unexpected, unintuitive, and undesirable invocations of - // our code that doesn't directly relate to rendering, including layers deeps like in a useEffect callback - // inside a custom React hook used by a custom component. - log("'no-op' indicated. The calling code is not actually interested in triggering a fetch request."); - return; - } - - if (state === "in-flight") { - log("Already in-flight."); - return; - } - - if (state === "untriggered") { - // This is not idiomatic. I'm confused, but I'm going to carry on so I can "learn by doing". - log("Untriggered. Ready to execute the fetch request. Setting state to 'in-flight'."); - state = "in-flight"; // this reassignment is odd. revisit. - dispatch(setFetchState(state)); - } - - const { input, init } = fetchParams; - - // We need to wire in the abort signal to the user's fetch options. - const controller = new AbortController(); - const signal = controller.signal; - const options = { - ...init, - signal, - }; - let isMounted = true; - - // Execute the fetch and bail out if the component is unmounted or the request was aborted. - log("Executing the 'fetch' request..."); - fetch(input, options) - .then((res) => { - log("The 'fetch' request completed."); - if (signal.aborted) - return Promise.reject( - new Error("The request was aborted. Short-circuiting the remainder of the 'useFetch' hook."), - ); - if (!isMounted) - return Promise.reject( - new Error("The component is no longer mounted. Short-circuiting the remainder of the 'useFetch' hook."), - ); - - return res.json().then((json) => { - dispatch(setFetchState({ kind: "response", status: res.status, json })); - }); - }) - .catch((err) => { - log("The 'fetch' request failed.", { err }); - if (isMounted && !signal.aborted) { - dispatch(setFetchState(err)); - } - }); - - // UPDATE: This is the crux of my problems. React is going to frequently mount/unmount components for various - // reasons. There is value in understanding this lifecycle and understanding how your code relates to it, but - // the fact is that the recognizable structure of your application logic and application UI is powered by an - // unknowable system of interstitial invocations of renders/mounts/unmounts by React. That's why React's advice - // is "hey try to make things pure functions". But of course, that's not possible. Our interest in executing a - // fetch request (to validate the GitHub personal access token, for example) spans longer than the in-between - // moments of these mount/unmounts. If we were to abort the fetch request upon unmount, that's bad. That's - // premature. That's why the code is commented out. In fact, this whole hook is now useless because - // it was designed to handle abort signals, but now I've figured out this is not a useful feature. - // - // Interestingly, TanStack Query also alludes to this problem in their documentation on "Query Cancellation": https://tanstack.com/query/latest/docs/react/guides/query-cancellation - // It reads: - // - // By default, queries that unmount or become unused before their promises are resolved are not cancelled. - // This means that after the promise has resolved, the resulting data will be available in the cache. This is - // helpful if you've started receiving a query, but then unmount the component before it finishes. If you - // mount the component again and the query has not been garbage collected yet, data will be available. - // - // This is a "clean-up" function that React calls when the component is unmounted. - // return () => { - // log("The component is unmounting. Aborting the 'fetch' request.") - // isMounted = false - // controller.abort() - // } - }, [fetchParams]); - - return state; -} diff --git a/src/useToken.ts b/src/useToken.ts index 7742492..d93d844 100644 --- a/src/useToken.ts +++ b/src/useToken.ts @@ -1,6 +1,5 @@ import { Dispatch, SetStateAction, useEffect, useState } from "react"; -import { useFetch } from "./useFetch"; -import { TokenState, logger } from "./code"; +import { logger, TokenState } from "./code"; const log = logger("useToken"); @@ -12,13 +11,14 @@ const log = logger("useToken"); * gets used. If not, the hook waits on the user to enter a token in the UI. After a token is restored or entered, the * hook validates it using the GitHub API. The hook gives the validated token the backend via IPC to store it. * - * This is not working well as a custom React hook. It might be better as a component or just externalized to non-React - * code. I've struggled and learned some things. But I'm still not clear on the right way to do this. + * Modeling "restoring" on {@link TokenState} is not symmetrical to modeling "isFetching" inside this hook. Consider + * refactoring this. */ export function useToken(): [TokenState, Dispatch>] { log("Invoked."); const [token, setToken] = useState("restoring"); const [shouldStoreAfterValidation, setShouldStoreAfterValidation] = useState(false); + const [isFetching, setIsFetching] = useState(false); if (typeof token === "object") { log({ kind: token.kind }); } else if (token === "empty") { @@ -45,31 +45,68 @@ export function useToken(): [TokenState, Dispatch>] { }); }, [token]); - let fetchParams: { input: RequestInfo | URL; init?: RequestInit } | "no-op"; - - if (typeof token === "object" && (token.kind === "entered" || token.kind === "restored")) { - const query = ` + useEffect(() => { + if (typeof token === "object" && (token.kind === "entered" || token.kind === "restored")) { + setIsFetching(true); + const query = ` query { viewer { login } } `; - const options = { - method: "POST", - headers: { - "Content-Type": "application/json", - Authorization: `bearer ${token.token}`, - }, - body: JSON.stringify({ query }), - }; + const options = { + method: "POST", + headers: { + "Content-Type": "application/json", + Authorization: `bearer ${token.token}`, + }, + body: JSON.stringify({ query }), + }; + + const fetchParams = { input: "https://api.github.com/graphql", init: options }; + log("Executing the 'fetch' request..."); + fetch(fetchParams.input, fetchParams.init) + .then((res) => { + log("The 'fetch' request completed."); + if (res.status == 401) { + log("GitHub API responded with 401 Unauthorized. The token is invalid."); + setToken({ + kind: "invalid", + token: token.token, + }); + return; + } + + return res.json(); + }) - fetchParams = { input: "https://api.github.com/graphql", init: options }; - } else { - fetchParams = "no-op"; - } + .then((json) => { + const login = json["data"]["viewer"]["login"]; + log("The token was found to be valid. GitHub login: ", login); + + // Note: this is verbose and circuitous... + if (shouldStoreAfterValidation) { + setToken({ + kind: "storing", + token: token.token, + login: login, + }); + } else { + setToken({ + kind: "valid", + token: token.token, + login: login, + }); + } + }) + .catch((err) => { + log("The 'fetch' request failed.", { err }); + // TODO Set token state to an error so that the using component can render user-friendly error message + }); + } + }, [token]); - const fetched = useFetch(fetchParams); useEffect(() => { if (typeof token === "object" && token.kind === "storing") { window.api @@ -86,67 +123,5 @@ export function useToken(): [TokenState, Dispatch>] { } }, [token]); - if (fetched === "in-flight") { - log("The 'useFetch' hook is in-flight. We need to wait for it."); - return [token, setToken]; - } - - if (fetched === "untriggered") { - log("The 'useFetch' hook is untriggered. We need to wait until its triggered."); - return [token, setToken]; - } - - if (typeof token === "object" && (token.kind === "valid" || token.kind === "invalid")) { - log("The token has already finished the validation process."); - return [token, setToken]; - } - - if (typeof token === "object" && token.kind === "storing") { - log("The token is storing."); - return [token, setToken]; - } - - if (typeof token !== "object" || !(token.kind === "entered" || token.kind === "restored")) { - // This is nasty. I'm having difficulty composing my code with 'useEffect'. It's hard to satisfy the requirement of "hooks must be called at the top level and never in a conditional". - throw new Error( - "This should never happen. The token should be an object with kind 'entered' or 'restored' at this point.", - ); - } - - if (fetched instanceof Error) { - setToken({ - kind: "invalid", - token: token.token, - }); - return [token, setToken]; - } - - const { status, json } = fetched; - - if (status == 401) { - log("GitHub API responded with 401 Unauthorized. The token is invalid."); - setToken({ - kind: "invalid", - token: token.token, - }); - } else { - const login = json["data"]["viewer"]["login"]; - log("The token was found to be valid. GitHub login: ", login); - - // Note: this is verbose and circuitous... - if (shouldStoreAfterValidation) { - setToken({ - kind: "storing", - token: token.token, - login: login, - }); - } else { - setToken({ - kind: "valid", - token: token.token, - login: login, - }); - } - } return [token, setToken]; }