Code review aplikacji napisanej w Reakcie przez junior developera (część 1/10 - błędy)

Moim gościem był Mati, junior developer, który w ramach nauki stworzył aplikację - listę zakupów. W nagraniu wykonałem code review aplikacji i omówiłem błędy, które Mati zrobił jako początkujący programista. W następnych nagraniach pokażę, w jaki sposób dane Matiemu porady zastosować w praktyce. Zapraszam do oglądania!

Linki:

00:00:00 - wstęp
00:01:06 - omówienie aplikacji
00:15:24 - GIT
00:19:00 - style (css)
00:26:25 - TypeScript
00:28:04 - obsługa błędów
00:29:07 - architektura
00:41:39 - komunikacja z BE (api)
00:48:07 - tłumaczenia
00:57:01 - asety
00:59:22 - linter
01:09:13 - nazewnictwo plików
01:13:21 - testy
01:24:59 - komponenty
01:30:19 - nieużywany kod
01:33:36 - Husky
01:35:27 - formularze (formik/react hook form)
01:40:03 - dostępne projekty na GitHub
01:40:31 - zakończenie

[WadzioV]

mega material, mysle ze jakbym trafil na ten material bedac jeszcze juniorem to moje repa bylyby ze 100x lepsze podczas rekrutacji co mogloby przyspieszyc sam proces

[grzywn]

Super materiał, może oprawa graficzna nie jest idealna, ale merytoryka robi swoje. Nauczyłem się bardzo dużo i na pewno będę stosować w swoich projektach. Pozdrawiam.

[ukaszs]

Fajne

Mam uwago co do dyskusji o testach.

"Unit" w "unit test" to jest "unit of isolation". Ten unit może być większy albo mniejszy.
Faktycznie zwykle unit oznacza mały kawałek, a jak jest większy określa się to jako integrację. Ale ten podział jest dość uznaniowy.

Mockowanie wszystkiego vs używanie faktycznej implementacji pod spodem to dwa zupełnie inne podejścia z innymi plusami i minusami.
Mocki sprawiają że test testuje tylko kod danego kawałka. Ale de facto często nie testuje funkcjonalności bo testuje mocki i nie sprawdza czy zależności są poprawnie użyte.
(podbicie wersji zależności nie podbije mocków, test sprawdza wykorzystanie mocka ale nie sprawdza biznesowego zachowania)
Używanie faktycznych zależności sprawia że testuje się czy dany kawałek ma funkcjonalność z zależnościami które będzie miał na prod.
Ale test jest potencjalnie droższy w wykonaniu, i trudniej ustalić co jest przyczyną jak failuje.

Warto dobrać do faktycznego projektu - jak drogie są te testy i jaki poziom bezpieczeństwa zapewniają.

Tak samo coverage warto odnieść się do mitów o coverage. Coverage nie jest miarą jakości testów tylko wysokopoziomową miarą ilości kodu nie pokrytego testami.

- jak coverage jest < 100% to znaczy że są miejsca w kodzie które w ogóle się nie wykonują podczas testów, te miejsca mogłyby mieć "throw Error()" a i tak testy byłyby zielone
pytanie nie jest ile jest % tylko w których miejscach kodu są dziury w testach
- coverage 100% nie oznacza że w teście są sensowne dane wejściowe, sensowne asercje itp. itd.
nie są też pokryte sekwencje operacji bo tam jest za dużo permutacji
ale przynajmniej każdy if choć raz się wykonał podczas jakiegoś testu

[dikamilo]

Jeśli chodzi o eslinta to ja lubię tam wymuszać formatowanie importów, ich sortowanie, grupy, odstępy itp. Z doświadczenia wiem że każdy inaczej to formatuje albo kompletnie nie zwraca na to uwagi. Jeden dostanie importy z IDE z automatu, inny dopisze je gdzie mu się podoba a jeszcze ktos inny puści sobie sortowanje. No robi się taka sama sytuacja jak z brakiem prettifiera.

Co do poruszanego tematu z kontekstami, pomijając już "god object", dochodzi kwestia performance oraz rerenderow, braku useCallback itp. Komponent nasłuchujące np. tylko na wybrany obecnie język dostanie rerender gdy jakiś inny komponent doda produkt do listy zakupów bo zmieni się stan kontekstu itp.

Dodal bym też storybooka a nawet oparł niektóre testy np. integracyjne o niego. Raz że dostajemy wizualna dokumentację projektu, można to pokazać biznesowi, dwa możemy tam zdefiniować w Black boxie całe flow biznesowe poszczególnych funkcjonalności projektu które można przeklinać a nawet dodać testy wizualne itp.

[FunnyFenix]

Na froncie przede wszystkim w testach powinno się odzwierciedlać zachowanie użytkownika, a nie pisać niepotrzebne unity które sprawdzają czy w button można kliknąć. Mockowanie bibliotek których używamy na widoku brzmi jak kuriozum, co usera obchodzi, że przykładowo material-ui się wywalił? Jeżeli zmockujemy to test nam tego nie powie. Mockuje się tylko i wyłącznie zapytania do zewnętrznych serwisów. Na froncie powinno się głównie pisać testy integracyjne, testujące logikę biznesową, nawet twórca react-testing-library sugeruje coś takiego.

[MrJ-ihmw]

Nie bardzo zrozumiałem podejście do tłumaczeń, czy mógłbym prosić o jakiś artykuł co miał Pan na myśli? I ten fragment o komponentach również.

[MrJ-ihmw]

Chce przerobić całą serie, chciałem na początku poklikać sobie po aplikacji aby zrozumieć jak działa, sprawdziłem z linku w opisie i zaczęły się problemy:
- w readme są 2 dema podane (rozumiem, że chodzi o drugie)
- pasy podane w readme do zalogowania się nie działają (zarejestrowałem nowe konto przy pomocy 10minutemail)
- zmieniłem język na polski i przy dodawaniu gdzie mam wybierz jednostkę, mam zaznaczone piece (nieprzetłumaczone) i kiedy chce dodać produkt pisze, że trzeba wybrać jednostkę (nie może być wybrane piece)
- produkty się nie dodają
- kiedy przechodzimy z pierwszej strony na kategorie produktu i klikamy add something to wybrana jest kategoria pieczywo (bez znaczenia z jakiej kategorii przeszliśmy
- tryb ciemny ma moim zdaniem tragicznie dobrane barwy

Czyli ogólnie aplikacja jest totalnie broken

Ale widzę, że commity są z zeszłego miesiąca więc pewnie demo jest nie odświeżone

[Dadgrammer]

Pierwsze co mi się rzuciło to, where TypeScript?

Jak będę miał więcej czasu to przejrzę całość i jakiś feedbaczek poleci.

[piotrzawierucha]

Remy są dobre do czcionki i niektórych odsępów, np. między linijkami, natomiast pixele są jak najbardziej poprawne w przypadku np. paddingu czy niektórych marginesów. Gdy osoba niedowidząca zwiększy sobie font size do np 32px zamiast domyślnych 16, to gdy div ma padding w remach to gwałtownie zmieni jego wygląd i przestrzeń wewnątrz i na zewnątrz, dlatego ujednolicanie nie jest koniecznie dobrym rozwiązaniem, bo zależy czy chcemy daną wartość zwiększać czy zmniejszać wraz ze zmianą wielkości czcionki.

[maciej]

Bardzo pomocny code review, odpowiedziałeś na wiele pytań, które miałem w głowie. Jedna jeszcze rzecz mnie zastanawia, mianowicie Atomic Design. Czy jest to najlepsza możliwa architektura projektu w Reacie? Jeśli nie to czy możesz podesłać jakieś źródła jak powinno się poprawnie organizować projekt. Wiem, że to jest temat rzeka, ale chciałbym do swoich projektów stosować jak najlepszą metodę.

[Loczek-lmbj]

Polecicie podobny kanał o backendzie w node?

[fuukowatty]

Czy Nx do mniejszych projektow jest lepszy niz taki vite np.?

[coder_one]

Koniecznie większy font w IDE, żeby nie trzeba było mrużyć oczu do ekranu! :)

[EuropeanLord]

Trochę nie łapię proponowanego nazewnictwa plików typu foo-bar.hook.tsx albo foo-bar.style.ts. Przecież to w typowym projekcie reaktowym nie ma sensu bo zwracamy również duzą uwagę na katalogi, czyli mam np. shared/hooks/FooBar.ts, shared/hooks/FooBar2.ts itd. jaki sens ma doklejanie .hook do nazw plików które są w katalogu "hooks"? Dla mnie to jest anti-DRY. Do tego argument o zmienianiu nazw strzałkami mnie nie przekonuje, po pierwsze jak często zmieniamy nazwy plików, po drugie to znowu jakiś pattern pod konkretne IDE. Temat poruszony w wideo 1:10:00 .

Również style, translacje itd. dot. komponentu X powinny być w tym samym katalogu, i jest to rozwiązanie preferowane przez devów Reacta, bo się skaluje. Marcin w wideo zrobił private-route.component.js i jest w pupie, bo co jeśli routów będzie np. 50 i każdy ma jeszcze po 2 pliki zależne od siebie? 150 plików obok siebie bez żadnych katalogów? IMO to kolejny zły wzorzec, ja tego nie widzę w dużym projekcie.

Samo wideo bardzo dobre, chociaż warto byłoby podkreślać że część rozwiązań jest opinionated (jak to wyżej), nazewnictwo z angulara musi się sprawdzić, ale w moich projektach wprowadziłoby więcej szkody, niż pożytku.