Migrate to typescript

[rodrigojmr]

Hey! So I know this is a large PR, and you might not use typescript, so that's possibly annoying. I wanted to do this mostly for practice (didn't take long with a migrate tool) and just in case you keep working on this site, I believe types will be very useful. I didn't use to but now I love TS for projects of every size.

Other changes besides typescript:

• Pass searchInput state to SearchBar. Technically you were re-rendering the whole app when typing.
• keyCode is deprecated, and key === 'Enter' is way more legible
• Separated theme to its own file, also due to TS's complaining when adding mobileCard.

Hope it's useful, and if it's not, then at least I hope it's good for checking out some TS!

[GamerKingFaiz]

Thanks for this! ♥

I do use TypeScript at work and probably should've started this project out with it. I just bootstrapped this with Create React App and just started developing. I generally like the freedom of being able to just code freely in JavaScript, but do agree that TypeScript can be helpful.

I'll review this more thoroughly in the future and decide whether I want to go forward with it or not.

I appreciate the other non-TypeScript call outs as well. I'll definitely look into those too.

[rodrigojmr]

Sure! There's definitely an easy case to make that at this point it might be overkill, so I completely getcha.

[GamerKingFaiz]

Hey, thanks again for this PR.

I added the fixes regarding the re-rendering bug and deprecated keyCode.

I'm not sure I fully understand the use of useCallback() in SearchBar.js though. Removing them seems to work fine too. Do you mind explaining? 😁

The React compiler also throws the following warning.

WARNING in src\components\navbar\SearchBar.js
  Line 50:6:  React Hook useEffect has missing dependencies: 'handleSearch' and 'searchInput'. Either include them or remove the dependency array. If 'handleSearch' changes too often, find the parent component that defines it and wrap that definition in useCallback

So I'm not sure if wrapping handleSearch in App.js makes sense or not. And what the dependencies would be in this case.

[rodrigojmr]

I'm not sure I fully understand the use of useCallback() in SearchBar.js though. Removing them seems to work fine too. Do you mind explaining? 😁

Sure. Thinking about it twice, it may be unnecessary. We're not too concerned with re-renderings on OutlinedInput and ´IconButton, and even with the callback it'll change every timesearchInput` state changes. Feel free to remove.

WARNING in src\components\navbar\SearchBar.js
  Line 50:6:  React Hook useEffect has missing dependencies: 'handleSearch' and 'searchInput'. Either include them or remove the dependency array. If 'handleSearch' changes too often, find the parent component that defines it and wrap that definition in useCallback

That's because you specifically did that on mount useState to set a default searchInput. The missing dependencies can sometimes be ignored if you're intentionally doing things on mount only. But this comment does lead to something I missed, which is handleSearch in a callback. Otherwise, when App re-renders, handleSearch is re-defined and passed as a prop, making a re-render in SearchBar.

So I'm not sure if wrapping handleSearch in App.js makes sense or not. And what the dependencies would be in this case.

As it is, it should be fine. Though not sure what you mean by wrapping, do you mean to just have it in App.js? SearchBar will handle it's own state and it or any other component can call that function with a string and trigger search. So it should be fine for a small app.

[GamerKingFaiz]

But this comment does lead to something I missed, which is handleSearch in a callback.

So just doing the below, right? I'm not sure if searchResults need to be in the dependencies or not.

  const handleSearch = useCallback(
    (value) => {
      setLoading(true);
      hltbService.search(value).then((result) => {
        setLoading(false);
        setSearchResults(result);
      });
      window.gtag("event", "search", {
        search_term: value,
      });
    },
    [searchResults]
  );

Though not sure what you mean by wrapping, do you mean to just have it in App.js?

Sorry for the confusion, I meant wrapping handleSearch in a useCallback like in the code snippet above.

[rodrigojmr]

Ah, that makes sense. Well, just like this warning about the dependencies, the compiler should tell you if it needs any dependencies. In this case I don't think it needs any (setSearchResults and setLoadingTime are available on mount, and searchResults are not used inside the function) so you can leave it empty. You're just making sure it won't be redefined on re-rendering the app.

With callback:

https://user-images.githubusercontent.com/11283573/148274451-59b2ea22-0a50-4f07-8703-18b31be8932f.mp4

Without callback:

https://user-images.githubusercontent.com/11283573/148274472-d8883dbc-8ee2-48c2-82c6-c5cbdef3c009.mp4

(profiler tab from the react devtools extension)

In this tiny example the render times are pretty identical (on some refreshes the callback will be faster), it's not a big component so it might be early optimization. That's also because there's a hook re-rendering it anyway, but if it didn't you'd also have less re-renders just from the function being memoized. Whenever you pass a function without callback to another component, it's going to be redeclared and it's going to make the component receive it as a new prop.