1
\$\begingroup\$

I am learning typescript with React and come up with this small program that each button count how many time by clicking and record which button got click. Please help me to take a look if all the code are the best practice.

import * as React from "react";
import "./styles.css";

interface ButtonContainerProps {
  options: string[];
}
interface ButtonProps {
  value: string;
  setSelectedOption(value: string): any;
}

const Button: React.FunctionComponent<ButtonProps> = (props): JSX.Element => {
  const { value, setSelectedOption } = props;
  const [count, setCount] = React.useState(0);
  const clickHandler = React.useCallback(() => {
    setCount(count + 1);
    setSelectedOption(value);
  }, [count, value, setSelectedOption]);
  return <button onClick={clickHandler}>{`${value}: ${count}`}</button>;
};

const Scoreboard: React.FunctionComponent<{
  selectedOption: null | string;
}> = ({ selectedOption }): JSX.Element => {
  return (
    <h2>
      {selectedOption
        ? `The selected option is ${selectedOption}`
        : `Please click following button to make an option`}
    </h2>
  );
};

const ButtonContainer: React.FunctionComponent<ButtonContainerProps> = ({
  options
}): JSX.Element => {
  const [selectedOption, setSelectedOption] = React.useState<string | null>(
    null
  );
  return (
    <div>
      <Scoreboard selectedOption={selectedOption} />
      {options.map((option) => (
        <Button
          key={option}
          value={option}
          setSelectedOption={setSelectedOption}
        />
      ))}
    </div>
  );
};
export default function App() {
  return (
    <div className="App">
      <h1>Button Container</h1>
      <ButtonContainer options={["apple", "orange", "pear"]} />
    </div>
  );
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Please do not update or remove the code in your question after receiving answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question or can anonymize it after the fact. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Mast
    Nov 22, 2020 at 7:41

1 Answer 1

2
\$\begingroup\$

React.FunctionComponent probably isn't necessary - it's boilerplate for every single component you have, and arguably makes typings a bit worse, not better. See this article. For example, you could accidentally do:

<Scoreboard selectedOption={selectedOption}>{'The Scoreboard'}</Scoreboard>

without an error being thrown because every React.FunctionComponent is typed as taking children, even if they're not used.

To make the types more accurate, and to reduce the amount of boilerplate, consider using completely plain functions instead.

Declare variables close to where they'll be used The ButtonContainerProps is declared at the top of the script, but it isn't used until you scroll past 2 intermediate components - so if you see

const ButtonContainer: React.FunctionComponent<ButtonContainerProps> = ({

and you don't remember what ButtonContainerProps is, you'll have to scroll all the way back up to the top to see what it is. Or, even better:

If something is only going to be used once, consider whether it can be reasonably defined inline. For example:

const ButtonContainer: ({ options }: { options: string[] }) => {

I think that's plenty clear.

Let TS automatically infer return types when possible - unless explicitly typing a value is necessary to make the TS compile, or to make it easier for a reader to understand, you consider just leaving it off, like with the above ButtonContainer, since the boilerplate doesn't accomplish much. The capitalization of a component should make it clear that the function is a component that returns JSX.

Possible duplicate key It's probably pretty unlikely, but if the caller of ButtonContainer passes an array of strings that contains a duplicate, the <Button key={option} will result in a duplicate key, which is a problem. If the buttons are completely static in the DOM, I'd make the key be the index of the element in the options:

{options.map((option, i) => (
  <Button
    key={i}

Or filter out duplicates from the options before rendering.

value name? One might normally expect a variable named value to contain something from the user that might change, like something in an input box, or a count of something. Here, the value being passed to Button is actually a name or label connected to the button (and the use of value alongside a stateful count variable might seem odd). Consider renaming it to name or buttonLabel or something of the sort. (Probably not option, since it's not related to <option>)

useCallback is useful when optimizing performance and reducing unnecessary re-renders when passing the callback as a prop to a child component. But here, there's just a <button> (not a child component) which'll only change on click (won't update frequently enough for the overhead of a function call to matter at all). The useCallback is introducing complication without accomplishing anything useful here; I'd consider removing it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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