3
\$\begingroup\$

When you use React I think that each page uses the useState and useEffect hooks:

  1. Initiate a GET of data from the server, and render the page immediately without data
  2. Then (asynchronously) receive the requested data from the server, and set the data into the state which re-renders the page

Each page has a different server URL probably, as well as a different data type.

My question is:

  • Is it conventional to have a separate copy of the useState and useEffect hooks in each page definition?
  • Or is it preferable (because of DRY) to have only one instance of the useState and useEffect hook in the whole application?

I feel that the latter is neat and clever -- and more maintainable because there's no copy-and-pasting -- but I fear that it's too clever, too "WTF?", unconventional, and not what an experienced React developer expects to see.

To achieve the latter I found (see code below, to be reviewed) that I can wrap the hooks into a subroutine that's used/reused to render any/every page.

  • The method to get data from the server is passed in as a parameter named getData
  • The method to render the page is passed in as a parameter named getLayout
  • The data type (I'm using TypeScript) is a template parameter named TData

This basic simple idea is implemented in the useGetLayout0 hook.

Overloaded versions of this function support various additional complications:

  • A parameter may be passed to the getData function
  • A parameter may be passed to the getLayout function
  • A second parameter may be passed to the getData function

An additional wrinkle is newData -- that exists so that it can be passed into a page layout function, so that the page layout function can invoke it to get more data, e.g. so the page can load data incrementally.

I hope this makes sense. I've only shown the first two of the functions which invoke this hook method, actually there are as many as there are pages in the application.

Is this too clever, would it be better to have useState and useEffect, with different hard-coded (not parameterised) getData functions, copied/distributed/embedded into each page (e.g. each getLayout function)? Would that be what's conventional? Is there an idiomatic or best-practice way to do this?

Another "benefit" of this method is that the getLayout is decoupled from the getData -- i.e. the page layout function doesn't know how data is fetched -- that coupling is all in this App.tsx file which defines the routes i.e. the app's URLs.

Is there another way to encapsulate this functionality somehow, so that it's reused by every page instead of being copied into every page?

import React from 'react';
import * as ReactRouter from 'react-router-dom';
import { useLayout, Layout, loadingContents, loadingError } from './PageLayout';
import { Topbar } from './Topbar';
import { Login } from './Login';
import './App.css';
import * as I from "../data";
import * as IO from "../io";
import * as Page from "./Pages";
import * as R from "../shared/urls";
import { AppContext, useMe } from './AppContext';
import { config } from "../config"
import { loginUser } from "../io/mock";
import { ErrorMessage } from "./ErrorMessage";
import { NewDiscussion as NewDiscussionElement } from "./Editor";
import { History } from "history";
import { SearchInput } from "../shared/post";

/*
  This defines the App's routes
  and the context (like global data) which is available to any chld elements which it creates.
*/

const App: React.FunctionComponent = () => {
  // https://fettblog.eu/typescript-react/context/ and
  // https://reactjs.org/docs/context.html#updating-context-from-a-nested-component
  const autologin = config.autologin ? loginUser() : undefined;
  const [me, setMe] = React.useState<I.UserSummary | undefined>(autologin);

  document.title = `${config.appname}`;

  // plus https://reacttraining.com/react-router/web/api/BrowserRouter
  return (
    <AppContext.Provider value={{ me, setMe }}>
      <ReactRouter.BrowserRouter>
        <AppRoutes />
      </ReactRouter.BrowserRouter>
    </AppContext.Provider>
  );
}

const AppRoutes: React.FunctionComponent = () => {
  // https://reacttraining.com/react-router/web/api/Switch
  return (
    <React.Fragment>
      <Topbar />
      <ReactRouter.Switch>
        <ReactRouter.Route exact path="/index" component={SiteMap} />
        <ReactRouter.Route exact path="/" component={Home} />
        <ReactRouter.Route exact path="/home" component={Home} />
        <ReactRouter.Route exact path={R.route.login} component={Login} />
        <ReactRouter.Route exact path={R.route.siteMap} component={SiteMap} />
        <ReactRouter.Route exact path={R.route.discussions} component={Discussions} />
        <ReactRouter.Route exact path={R.route.newDiscussion} component={NewDiscussion} />
        <ReactRouter.Route exact path={R.route.users} component={Users} />
        <ReactRouter.Route exact path={R.route.tags} component={Tags} />
        <ReactRouter.Route path={R.route.discussionsTagged} component={Discussions} />
        <ReactRouter.Route path={R.route.users} component={User} />
        <ReactRouter.Route path={R.route.images} component={Image} />
        <ReactRouter.Route path={R.route.discussions} component={Discussion} />
        <ReactRouter.Route path={R.route.tags} component={Tag} />
        <ReactRouter.Route component={NoMatch} />
      </ReactRouter.Switch>
    </React.Fragment>
  );
}

type RouteComponentProps = ReactRouter.RouteComponentProps<any>;

/*
  This is a "high-order component", a "custom hook" -- it separates "getting" the data from "presenting" the data.

  - https://reactjs.org/docs/higher-order-components.html
  - https://reactjs.org/docs/hooks-custom.html

  The sequence of events is:

  1. Called for the first time
  2. Returns hard-coded `renderLayout(loadingContents)` which displays a "Loading..." message
  3. useEffect fires and:
     - Call getData to fetch data from the server
     - Call getContents to render the data into a Layout instance
     - Call renderLayout again to show the calculated Layout elements

  The renderLayout method support different page layouts --
  e.g. narrow text, full-screen images, a grid, and with optional extra columns.

  To support this it's convenient to make a single hard-coded call to renderLayout in any case,
  but to declare its input parameter type (i.e. the Layout interface) to be flexible/expressive,
  so that the getContents (i.e. one of the Page functions) can define arbitrarily complex content and layout.

  - getContents defines the contents of the page by creating a Layout instance which contains elements
  - renderLayout defines the page's columns within which the elements in the Layout are rendered

  ---

  Fetching data is as described at:

  - https://reactjs.org/docs/hooks-faq.html#how-can-i-do-data-fetching-with-hooks
  - https://overreacted.io/a-complete-guide-to-useeffect/
  - https://www.robinwieruch.de/react-hooks-fetch-data

  And using a hook with TypeScript:

  - https://www.carlrippon.com/typed-usestate-with-typescript/

  The template supports a parameter of type TParam (which is optional and may be void/undefined).
  If specified then the parameter is passed to the getData function and to the getContents function.

  ---

  Also, as described here ...

  https://stackoverflow.com/questions/56096560/avoid-old-data-when-using-useeffect-to-fetch-data

  ... if the parameter value changes then there's a brief wndow before the useEffect hook is run.
  Therefore the param value is stored in state whenever the data value is stored,
  and the data value is discarded when it's associated param value doesn't match the current param value.

  The solution described here ...

  https://overreacted.io/a-complete-guide-to-useeffect/#but-i-cant-put-this-function-inside-an-effect

  ... i.e. to "wrap it into the useCallback Hook" was insufficient because it leaves a brief
  timing hole before the useEffect fires and the data is refetched.
*/

// this gets data from the server
type IoGetDataT<TData, TParam, TParam2 = void> = (param: TParam, param2?: TParam2) => Promise<TData>;

// this defines two exra functions (named `reload` and `newData`) which are passed to the `getLayout` function
type Extra<TParam> = { reload: () => void, newData: (param: TParam) => Promise<void> };

// this uses data from the server, and optional extra data, to create a Layout object
type GetLayoutT<TData, TExtra, TParam> = (data: TData, extra: TExtra & Extra<TParam>) => Layout;

// this value is passed as param to useGetLayout when TParam is void
// or I could have implemented a copy-and-paste of useGetLayout without the TParam
const isVoid: void = (() => { })();

// 1st overload, used when TParam is void
function useGetLayout0<TData>(
  getData: IoGetDataT<TData, void>,
  getLayout: GetLayoutT<TData, {}, void>): React.ReactElement {
  return useGetLayout<TData, void>(getData, getLayout, isVoid);
}

// 2nd overload, used when TParam (passed to the IO function) is significant
function useGetLayout<TData, TParam>(
  getData: IoGetDataT<TData, TParam>,
  getLayout: GetLayoutT<TData, {}, void>,
  param: TParam): React.ReactElement {
  return useGetLayout2<TData, TParam, {}>(getData, getLayout, param, {});
}

// 3rd overload when there's TExtra parameter data to pass to the page layout function
function useGetLayout2<TData, TParam, TExtra extends {}>(
  getData: IoGetDataT<TData, TParam>,
  getLayout: GetLayoutT<TData, TExtra, void>,
  param: TParam,
  extra: TExtra)
  : React.ReactElement {
  return useGetLayout3<TData, TParam, TExtra, void>(getData, getLayout, param, extra);
}

// 4th overload when there's a second TParam2 parameter passed to the IO function
function useGetLayout3<TData, TParam, TExtra extends {}, TParam2>(
  getData: IoGetDataT<TData, TParam, TParam2>,
  getLayout: GetLayoutT<TData, TExtra, TParam2>,
  param: TParam,
  extra: TExtra)
  : React.ReactElement {

  const [prev, setParam] = React.useState<TParam | undefined>(undefined);
  const [data, setData] = React.useState<TData | undefined>(undefined);
  const [error, setError] = React.useState<Error | undefined>(undefined);

  // we pass the reload function to the getLayout function so that it can force a reload e.g. after
  // posting a new message to the server. We force a reload because nothing has changed on the client --
  // not even the URL -- but we want to fetch/refresh the data from the server.
  // https://stackoverflow.com/questions/46240647/can-i-call-forceupdate-in-stateless-component
  const [toggle, setToggle] = React.useState<boolean>(true);
  function reload() {
    setToggle(!toggle); // toggle the state to force render
  }

  // we pass a newData function to the getLayout function so that it can invoke the network I/O function again
  // with a new parameter (see the ThrottledInput function) and store the new data and the new parameter back here
  const newData = React.useMemo(() => {
    const getDataAgain: (param2: TParam2) => Promise<void> = (param2: TParam2) => {
      const promise = getData(param, param2);
      const rc: Promise<void> = new Promise<void>((resolve, reject) => {
        promise.then((fetched: TData) => {
          // the layout function has fetched new data with a new parameter
          // so redo now what was originally done at the end of useEffect
          setData(fetched);
          // setParam(param);
          resolve();
        })
        promise.catch(error => {
          reject(error);
        });
      });
      return rc;
    }
    return getDataAgain;
  }, [getData, param]);

  // add the reload function to the extra data which we pass as a parameter to the layout function
  // so that the layout function can call reload() if it wants to
  const extra2: TExtra & Extra<TParam2> = { ...extra, reload, newData };

  React.useEffect(() => {
    getData(param)
      .then((fetched) => {
        setData(fetched);
        setParam(param);
      }).catch((reason) => {
        console.log(`useEffect failed ${reason}`);
        setError(reason);
      });
  }, [getData, getLayout, param, toggle]);

  // TODO https://www.robinwieruch.de/react-hooks-fetch-data/#react-hooks-abort-data-fetching

  const layout: Layout = (data) && (prev === param)
    ? getLayout(data, extra2) // render the data
    : (error)
      ? loadingError(error)
      : loadingContents; // else no data yet to render

  return useLayout(layout);
}

/*
  These are page definitions, which have a similar basic structure:

  - Invoked as a route from AppRoutes
  - Delegate to useGetLayout

  There's a different function for each "route" -- i.e. for each type of URL -- i.e. each type of page data and layout.
*/

const SiteMap: React.FunctionComponent = () => {

  return useGetLayout0<I.SiteMap>(
    IO.getSiteMap,
    Page.SiteMap
  );
}

// these are used as TExtra types
type FetchedIsHtml = { isHtml: boolean };

const Home: React.FunctionComponent = () => {

  const isHtml = false;
  const filename = isHtml ? "home.html" : "home.md";

  return useGetLayout2<string, string, FetchedIsHtml>(
    IO.getPublic,
    Page.Fetched,
    filename,
    { isHtml }
  );
}
\$\endgroup\$
3
\$\begingroup\$

Is this too clever, would it be better to have useState and useEffect, with different hard-coded (not parameterised) getData functions, copied/distributed/embedded into each page (e.g. each getLayout function)? Would that be what's conventional? Is there an idiomatic or best-practice way to do this?

First things fist. Whenever you start using multiple useState and useEffect calls, make sure you keep the state of hooks between those calls in mind. See the Rules of Hooks. The bigger your code gets, the easier it is to accidentally break this. To help with this, make sure you keep your code readable so it's easier to spot those mistakes at first glance. The order of the hooks is important and this gets trickier with conditionals.

I don't know much about React, but what little of it I do know is that it encourages to split-up. Files, functions, basically everything should be as small as it could be. Which style (one per page vs one per app) do you think fits this philosophy best? Could you split-out the functionality to its own function, its own file even? Do you ever need to run multiple effects on the same page? Do you ever need to skip effects for performance reasons?

For sources on splitting-up as being encouraged, see:

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ One file per class seems like a JS thing more than a React thing. All the cool frameworks tutorials, I've read, follow it and so do most of their tools. \$\endgroup\$ – Peilonrayz Jun 11 at 0:25
4
\$\begingroup\$

I don't normally code in JavaScript but here's some problems I see with the code.

  • When skimming your code it's hard to tell where a lot of function's argument and body, end and begin. this is because all arguments, return type and function body are at the same level of indentation.

    function useGetLayout2<TData, TParam, TExtra extends {}>(
      getData: IoGetDataT<TData, TParam>,
      getLayout: GetLayoutT<TData, TExtra, void>,
      param: TParam,
      extra: TExtra)
      : React.ReactElement {
      return useGetLayout3<TData, TParam, TExtra, void>(getData, getLayout, param, extra);
    }
    

    Whilst you're consistent with this problem, you're not consistent in how it manifests. I would at the very least pick one form and stick with it.

    In addition to the above it's easy to misread the return type of functions as part of the type of the last argument. Take:

    function useGetLayout0<TData>(
      getData: IoGetDataT<TData, void>,
      getLayout: GetLayoutT<TData, {}, void>): React.ReactElement {
      return useGetLayout<TData, void>(getData, getLayout, isVoid);
    }
    

    Contrast with:

    function useGetLayout0<TData>(
      getData: IoGetDataT<TData, void>,
      getLayout: GetLayoutT<TData, {}, void>,
    ): React.ReactElement {
      return useGetLayout<TData, void>(getData, getLayout, isVoid);
    }
    

    The trailing comma helps with Git diffs when you need to add another argument to a function.

  • I'm not a fan of how you call React.useMemo, in no way does it look like the lambda is not the only argument to it.

    • You have not put the start of the lambda on a new line from the opening bracket of the function.
    • You have not indented the body of the lambda to show that there are other arguments at the same level.
    const newData = React.useMemo(() => {
      ...
    }, [getData, param]);
    
    const newData = React.useMemo(
      () => {
        ...
      },
      [getData, param],
    );
    

    The second form allows for more functions to be passed to the function without the need for janky formatting of the arguments.

    const newData = React.useMemo(() => {
      ...
    }, [getData, param], () => {
      ...
    });
    
  • I'm not a fan of how you're getting newData in useGetLayout3:

    • I'm not a fan of making const variable and then just return variable the next line.
    • Normally there's never a need to manually make a Promise. I've not used JS in a while but I'm pretty sure you can just use getData(...).then(fetched => {setData(fetched)})

    const newData = React.useMemo(
      () => (param2: TParam2) => getData(param, param2).then(f => {setData(f)}),
      [getData, param],
    );
    
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Changing the automated code formatter to "Prettier" implemented some of your suggestions (I was using the default formatter built-in to VS Code). \$\endgroup\$ – ChrisW May 2 at 19:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.