React

Practices and patterns for React

Components

Functional Component Definitions

While there are a few ways to define a function component, this is the preferred way in the Illust codebase:

export interface MyCompProps {
  value: string;
}

export const MyComp: FunctionComponent<MyCompProps> = ({ value }) => {

Rest props

If a React component returns a simple DOM element like a button or text, the calling component may want to be able to set that element's position, margins, etc. You could encode the margins in the child component (which makes it hard to re-use), or add a specific prop for margins (what if later you want to re-use that component elsewhere, and make it absolutely positioned?). Instead, try including the Prop type of the returned element in the props of the wrapping component, and passing the props to the element. That way, the calling component can set whatever display values it needs to, without having to change the child component definition.

This also lets you set default values for element props (by putting them before the spread props, like with color in the example), but have the calling component override them. And you can enforce a prop that can't be overridden by adding it after the spread (like onClick in the example).

For example, a button that triggers a modal:

interface MyButtonProps extends ButtonProps {
  contents: ReactNode;
}

const MyButton = ({ contents, ...props }) => {
  const { isOpen, onOpen, onClose } = useDisclosure();
  return (
    <>
      <Button color="black" {...props} onClick={onOpen} />
      <Modal isOpen={isOpen} onClose={onClose}>
        {contents}
      </Modal>
    </>
  );
};

Because MyButton accepts any of the ButtonProps, it's easy to set the button's margins, and override its color if need be:

const MyContainer = () => {
  return (
    <MyButton marginTop={3} color="blue" contents={<Text>Hello world</Text>}>
      Click Me
    </MyButton>
  );
};

Omitting Props

One issue with this approach is when you want a prop that has a different name as an existing element prop, but a different type. For instance, and onClick handler that accepts a value instead of an Event. For this, you can use the Omit type from typescript to remove specific types from the element's props:

interface AnswerButtonProps extends Omit<ButtonProps, "onClick" | "type"> {
  // With out the Omit, you get:
  // Type '(value: "accept" | "deny") => void' is not assignable to type 'MouseEventHandler
  onClick: (value: "accept" | "deny") => void;
  // With out the Omit, you get:
  // "accept" | "deny"' is not assignable to type '"button" | "reset" | "submit"'
  type: "accept" | "deny";
}

const AnswerButton: FunctionComponent<AnswerButtonProps> = ({
  onClick,
  type,
  ...props
}) => {
  return (
    <Button {...props} onClick={() => onClick(type)}>
      {type === "accept" ? "Accept" : "Deny"}
    </Button>
  );
};

Callback prop naming

In general, callback props should be prefixed with "on" (like onClick, and callbacks declared in a component should be prefixed with handle (like handleClick). This has a couple benefits:

• when looking at the props passed into a component it makes it clear which props should be callbacks

• it prevents name collisions when wrapping an incoming prop in another function (like const handleChange = (event) => onChange(event.target.value)

Early Returns

It's common for a React component to have some state (loading, invalid data, etc.) that results in it either not rendering at all, or just rendering a spinner. In those cases, it can be easier to follow the logic of the component with an early return:

cosnt MyComp = ({ loading, data }) => {
  return loading ? (
    <div>
     <div>
       <div>
         {data}
       </div>
     </div>
    </div>
  // You eyes have to go to the bottom of the function to see what happens when loading is false
  ) : null;
}
// Early return
cosnt MyComp = ({ loading, data }) => {
  // The logic is a little easier to follow, becase the two cases are both
  // right next to eachother.
  if (!loading) return null;
  return (
    <div>
     <div>
       <div>
         {data}
       </div>
     </div>
    </div>
  );

This doesn't work if the condition is inside some JSX - but for a large enough component, a chunk of markup that is conditionally rendered is a good candidate for extraction to a new component, which then can do the early return, or be wrapped in a short conditional in the parent component. You may also find that by extracting this conditional markup, some hooks and other calculations may also now move to the extracted component.

cosnt MyComp = ({ loading, data }) => {
  return (
    <div>
      // Lots of nesting
      {loading && (
       <div>
         <div>
           {data}
         </div>
       </div>
      )}
    </div>
  ) : null;
}
// Vs. with extraction
cosnt MyComp = ({ loading, data }) => {
  return (
    <div>
      <MyLoadedComp loading={loading} data={data} />
    </div>
  ) : null;
}

cosnt MyLoadedComp = ({ loading, data }) => {
  if (!loading) return null;
  return (
     <div>
       <div>
         {data}
       </div>
     </div>
  );
}

External links

Add rel="noopener noreferrer" to links to with target="_blank". noopener and noreferrer both solve some security concerns.

Chakra

No unstyled CSS classes

Chakra's classes aren't very human-readable, so it's useful while debugging to add CSS classes & ids to elements. However, if these get merged into the codebase, they may make future deveopers think those classes are actually styled.

So, any CSS classes or ids that don't have an effect on the code should be removed before merging.

Hooks

Combine multiple hooks into a custom Hooks

When a component has multiple hooks that interact with each other (like a useState that is populated by a useEffect, that's a sign that the collection of related hooks could be pulled into a custom hook that returns the state. For example, imagine a simple component that loads an ArtPiece from the database, and then allows the user to edit that piece. Instead of putting all the hooks in the component, like so:

const ShowArtPiece = ({ id }) => {
  const [artPiece, setArtPiece] = useState([]);
  const [isLoading, setIsLoading] = useState(true);
  
  useEffect(() => {
    return db.artPieces.doc(id).onSnapshot(snapshot => {
      setArtPiece(snapshot.data)
      setIsLoading(true);
    });
  }, [id]);
  
  return (<Box>
    {artPiece.name}
    <ArtPieceEditor onChange={setArtPiece} />
  </Box>)
}

You could instead pull the fetching logic into a hook, which isolates the fetching logic and makes it easier to test, and re-use elsewhere in the codebase, while also keeping the component definition itself focused on presentation logic:

const ShowArtPiece = ({ id }) => {
  const [artPiece, isLoading, setArtPiece] = useArtPiece(id);
  
  return (<Box>
    {artPiece.name}
    <ArtPieceEditor onChange={setArtPiece} />
  </Box>)
}

const useArtPiece = (id) => {
  const [artPiece, setArtPiece] = useState([]);
  const [isLoading, setIsLoading] = useState(true);
  
  useEffect(() => {
    return db.artPieces.doc(id).onSnapshot(snapshot => {
      setArtPiece(snapshot.data)
      setIsLoading(true);
    });
  }, [id]);
  
  return [artPiece, isLoading, setArtPiece] as const;
}

This also limits the scope of some variables that the main component doesn't care about (like setIsLoading in this case) to just the hook, instead of making the reader of the component check if that could also be called in an onClick event handler or something.

Don't use unnecessary useMemo/useCallback/useState

useMemo/useCallback should generally only be used for stopping a useEffect from running unnecessarily, and not as a performance optimization unless the React profiler shows a problem. The same goes for useState - it shouldn't be used to cache a value that can be cheaply re-calculated on every render loop.

// Unnecessary useMemo
const MyComponent = ({ list }) => {
  const firstItem = useMemo(() => list[0], [list]);
}

// Unnecessary useState forking
const MyComponent = ({ list }) => {
  const [firstItem , setFirstItem] = useState(null);
  useEffect(() => {
    setFirstItem(list[0]);
  }, [list]);
}

// This is much easier to follow, and also has less overhead
const MyComponent = ({ list }) => {
  const firstItem = list[0];
}