Cat application

[viktorntaf]

This the task that was asked regarding a user seeing cat images, info for each cat, and adding favorite cat images.

Notes
While this a working example with 100% of the functionality there is still room for improvements. Some them are:

• Global error wrapper to ensure our application would not fail unexpectedly.
• Common functions to handle API errors (both HTTP errors and common errors produced by all APIs)
• More global loaders(page loader, image loader etc)
• Consider add a wrapper on MUI so it would be easier to update library in a future breaking change.
• Move x-api-key to a configuration file in order to be easier to change it on the spot

[chbardamu]

@viktorntaf what made you use JS over TS? And since you decided to go with that, why not using some basic error handling to counterbalance the lack of type checking?

[chbardamu]

Move x-api-key to a configuration file in order to be easier to change it on the spot

If you had to design an actual app that uses such a key, how would you approach that?

[chbardamu]

What kind of improvement could you do with this component?

[viktorntaf]

1. className=".MuiToolbar-root" is a mistake since it's not a valid React className, and it's redundant because MUI provides this class by default.
2. Inline styling isn't a good choice; I would use the sx prop provided by MUI, which I use throughout the rest of the app.

[chbardamu]

Great, I'd also add that it could be moved to a separate file.

[chbardamu]

Why not just use that in the App component instead of that "div" you used as wrapper?

[viktorntaf]

I use const location = useLocation(); that needs to be inside a context. So I split the component in order for the useLocation to have access to it.

[chbardamu]

Got it. Still, this JSX structure looks a bit odd. I haven't followed on the latest react-router version and if it's still supported by the current API, but maybe moving <NavBar /> inside <Route path="/images/:id"> as child would do the trick, or even accessing window.location.path directly.

[viktorntaf]

I agree, it does feel a bit off. To achieve the same functionality using the children approach, I’d need to place <NavBar /> inside every route except <Route path="/images/:id">, ensuring it appears on all pages but the modal. However, that would require repeating this for every new route, which isn't ideal for maintainability.

A better alternative would be moving useLocation inside <NavBar /> to handle conditional rendering directly within the component. This would keep the App.js structure more traditional and cleaner. However, this change would turn the <NavBar /> from a presentational to a functional component, making it more specific to this app's routing logic. As a result, it would lose reusability across other parts of the app, since it’s no longer a general-purpose component.

While this approach sacrifices some flexibility, I agree it could be worth it for simplicity in this specific case.

[chbardamu]

Do you really need this useEffect here?

[chbardamu]

And why do you need this extra isFavourite in your component's state when you already have this information (catIsFavourite)?

[viktorntaf]

The API response catIsFavourite indicates if the user has marked the cat image as a favorite, while isFavorite is an internal component state used to toggle the checkbox. I could have named it better, like userHasCheckedFavorite. I designed it this way to avoid calling the isCatFavourite API each time the checkbox is toggled, optimizing performance.

[viktorntaf]

Do you really need this useEffect here?

You are correct we can avoid the useEffect here if checkbox is a different component. That way we control the renders of the new checkbox component, thus it's internal state, by passing catIsFavourite?.length as a prop.

[chbardamu]

Why did you decide to use axios over the native fetch API?

[viktorntaf]

I chose axios for its built-in support for timeouts, interceptors, and error handling. It also simplifies tasks like setting headers and parsing JSON. While these features may not be essential for a simple app, I selected it with scalability in mind for future growth.

[chbardamu]

Fair enough. But are any of these features necessary in your app? I'm trying to understand the gains over, say, ending up with a slightly larger output/bundle file after compiling to JS.

[viktorntaf]

In the current project it is true that is not needed and I initially used only fetch. I made the switch to axios because:

• I called POST:https://api.thecatapi.com/v1/favourites with an existing image id and it returned 400. To me that was an indication that the handling error functionality from AXIOS would come in handy in order to implement both a generic errors handler and specific requests like treating a 400 as non-fatal
• Another reason for switching was the POST to https://api.thecatapi.com/v1/images/search, which returned more data than needed. Using Axios’s transformResponse would allow to filter the response before passing it to the component.

Also i since it is fairly lightweight in bundle size (35.8 kB Minified) I believed that the future benefits would overscale the extra bundle size.

To be fair, Axios isn't really needed for this project as said earlier, and if I followed the YAGNI approach, I wouldn't have used it. However, I chose it with future growth in mind, anticipating the app will scale and benefit from its features later on.

[chbardamu]

I can't help it but wonder why using react-query when you opted for a minimal approach that left out the list of improvements you included in your PR's opening comment.

[viktorntaf]

Initially, I didn’t plan to use react-query, but the need for a paginated query on the dashboard made me reconsider, as it provides an out-of-the-box solution for managing server state and pagination. Additionally, when I started with Next.js, react-query was one of the suggested libraries, which influenced my decision.

[viktorntaf]

@viktorntaf what made you use JS over TS? And since you decided to go with that, why not using some basic error handling to counterbalance the lack of type checking?

I initially started with Next.js, a React framework, and TypeScript for SSR benefits (e.g., the favorites page) and potential SEO needs. On Friday, I got feedback that while you'd be okay with Next.js, it wasn’t your preference.
So, I switched to React and JavaScript since I’m more familiar with it and wanted to focus on functionality. I didn’t implement prop definitions or a global error boundary due to time constraints.

In hindsight, I should’ve stuck with Next.js or requested more time.

[viktorntaf]

If you had to design an actual app that uses such a key, how would you approach that?

I’d store the API key in a .env file for local development, and in a cloud environment, it would live in environment variables. This ensures different keys across environments (dev, QA, prod) and enhances security by not exposing the key in the code. We can also change the key without needing code changes or PRs, making the app more scalable and versatile.

[chbardamu]

@viktorntaf what made you use JS over TS? And since you decided to go with that, why not using some basic error handling to counterbalance the lack of type checking?

I initially started with Next.js, a React framework, and TypeScript for SSR benefits (e.g., the favorites page) and potential SEO needs. On Friday, I got feedback that while you'd be okay with Next.js, it wasn’t your preference. So, I switched to React and JavaScript since I’m more familiar with it and wanted to focus on functionality. I didn’t implement prop definitions or a global error boundary due to time constraints.

In hindsight, I should’ve stuck with Next.js or requested more time.

I see - this covers not using SSR, but TS could be used regardless that, no?

I noticed you used annotations in some parts, but they're incomplete and partial to offer type guidance.

[viktorntaf]

I see - this covers not using SSR, but TS could be used regardless that, no?
I noticed you used annotations in some parts, but they're incomplete and partial to offer type guidance.

I completely agree with you—it really was just the time constraint that led me to choose JavaScript over TypeScript. In hindsight, it probably wasn’t the best decision, especially since I had already built half the app in TypeScript with Next.js. Implementing TypeScript in the React version likely wouldn’t have taken much more time, and I could have benefited from the type safety it offers.