Code Review: TMDb React App

Cześć,
może znajdzie się tutaj ktoś, kto chciałby rzucić okiem na moje repo:

https://github.com/danzawadzki/the-movie-db-react-app

Prosta aplikacja napisana z wykorzystaniem react-a. Po wpisaniu czegoś w wyszukiwarkę (po zakończeniu pisania) wysyłam request do api TMDb z prośbą o listę pasujących wyników. Sortuje po tych które mają najwięcej głosów (zakładam, że to popularność oceny, będzie najlepszą miarę prawdopodobieństwa) i wyświetlam listę 10 najlepszych propozycji. Po kliku w film z dropdowna, wysyłam kolejny request o detale filmu i podmieniam w stanie aplikacji.
Bez reduxa. Bootstrap dla ulatwienia stylowania (style podzieliłem na źródłowe, które są dla mnie niemutowalne oraz te w folderze custom). Link do wersji live w opisie (na gh-pages). Aplikacja na moje oko działa, ale chciałbym dostać od Was feedback nad czym popracować, jakich praktyk unikać, a jakie mógłbym poznać :)/

Mam nadzieję, że znajdzie się ktoś taki pomocny!

Może przerzuć wszystko na nowego brancha, przesuń mastera na initial commit i wystaw pull request to będzie można zrobić review dla całego kodu na githubie (inaczej można chyba tylko do poszczególnych commitów). To chyba będzie coś mniej więcej takiego:

git checkout origin/master
git branch nowy
git push -u origin nowy
git checkout master
git reset --hard 9a2ac96fc56a55d0d1ce936762f596d736bea3bb
git push -f

EDIT: Zrobiłem to za ciebie:

https://github.com/BartekLew/the-movie-db-react-app/pull/1

Ło panie, a co tam przykładowy film zahardkodowany w kodzie robi??

Poza tym tak na szybko:

• brakuje PropTypes wszędzie,
• poczytaj sobie o container i presentional components, bo klasa App u Ciebie robi wszystko - fetchuje dane, przetwarza je i prezentuje wynik,
• zamiast stosować jakieś ad-hoc komentarze zainteresuj się JSDoc,
• zainwestuj w ESLinta,
• polecam używać destructuringu propsów,
• testy to masz obszerne :D
• CSSy na pałe pisane - mez żadnej metodologii,
• nie sklejaj w Sass nazw klas za pomoca & bo kod jest przez to nieczytelny (trzeba skakać wzrokiem) i ciężko się debuguje - bo kopiujesz sobie nazwe klasy w dev toolsach i jak to potem wyszukac w kodzie?

Sama apka nawet spoko, prosta i estetyczna (tylko dodaj cursor: pointer na liście wyników wyszukiwania) ;)

Burget?

EDIT:
Na moje oko jedyny plus, że działa. Całość wygląda jakby React przeszkadzał Ci w pisaniu.

• tasiemiec w this.state
• constructor(props), po co przekazujesz propsy skoro ich nie używasz?
• tenary operator zabija czytelność przy takich kobyłach.

 keyWord.length > 0 ? (
            fetch(`https://api.themoviedb.org/3/search/movie?api_key=${Config.apiKey}&query=${keyWord}`)
                .then(response => response.json())
                .then(json => {
                    json.results = json.results
                        .sort((a, b) =>
                            (a.vote_count > b.vote_count) ? -1 : 1);
                    this.setState({autocompleteSuggestions: json});
                })
        ) : this.setState({autocompleteSuggestions: Object.assign({})});
    }

• Jeżeli musisz się tak wspomagać znaczy, że masz zła strukturę komponentów

document.querySelector(".searchengine-input").value = props.movie.title;
const root = document.querySelector("#root");

• po kodzie można wnosić, że masz zerową wiedzę na temat cyklu życia komponentu (the component lifecycle)
• największy błąd to wrzucenie praktycznie wszystkiego do jednej klasy, no i jeszcze ten film w this.state

@mechanix:
@Maciej Cąderek

Wiem, że poświęciliście mi już wczoraj sporo czasu i jestem za to niesamowicie wdzięczny, ale bardzo zależy mi, żebyście rzucili okiem jeszcze raz. Zrobiłem istotnych kilka zmian:

1. Rozbiłem sporo komponentów na mniejsze i odseparowałem, te które uważam za kontenery od tych które uważam za czysto prezentacyjne.
2. Wiem jak wygląda cykl życia komponentu, ale myślałem (błędnie), że wykorzystanie w tym przypadku nie będzie tak istotne. Ostatecznie przerobiłem całość. Przyznaje, że to rozwiązanie jest znacznie elegantsze.
3. Wywaliłem kobyłę ze stanu i teraz po zamontowaniu aplikacji wysyłam request o detale filmu (id pierwszego filmu wciąż jest stałe, ale to zdecydowanie mniejsze zło).
4. Wywaliłem props-y w miejscach, gdzie faktycznie ich nie wykorzystuję.
5. Przystopowałem nadużywanie operatora tenary.
6. Wywaliłem wspomaganie selector-ami. Teraz stan input-a zależy wyłącznie od stanu jego rodzica. Nic więcej.
7. Dorzuciłem folder z modułami do fetch-owania danych z API, z config-iem (klucz i tak pokazuje przy request-ach, wiec go nie ukrywalem) i handler-ami.
8. Przy dzieleniu na moduły zawęziłem zakres odpowiedzialności, tj. funkcja pobierająca dane z api, tylko pobiera dane. Obróbką zajmuje się gdzie indziej (funkcja callback). Powinno podnieść re-używalność kodu.
9. Przerobiłem moje "wesołe" komentarze na ustandaryzowane JSDoc (z tym jeszcze trochę walczę).
10. Wywaliłem z repo zbudowaną wersję (tę mam tylko na gh-pages).
11. Wywaliłem kilka linii dzięki destructuring-owi.

Muszę jeszcze przemyśleć jak zachować się przy nieudanym request-cie. Dorzucić jakieś loader-y przy ładowaniu obrazków. Przerobić css-y. Dodać propTypes. I znacznie, znacznie więcej. Z tym, że pierw chce poprawić to co najważniejsze.

Żeby nie było, że wybiórczo zająłem się tylko częścią rzeczy. Zrobię wszystko (z resztą już zacząłem :)). Ale na tyle wystarczyło mi dziś czasu, a sama struktura aplikacji wydaje mi się priorytetowa w tym momencie :D. BEM już szlifuję.

Nadal masz skopany komponent App - nie masz tam podziału na logikę i prezentację, łap może przykład o co chodzi:

/**
 * Presentational component:
 * "dumb", knows nothing about external world,
 * can have methods and state only for presentation purpose
 */
const Post = ({ title, content, fetchNext }) => {
  return (
    <div>
      <button onClick={fetchNext}>next</button>
      <h1>{title}</h1>
      <p>{content}</p>
    </div>
  )
}

/**
 * Container component:
 * "smart" - handles data fetching (from store, from API etc.),
 * does not describe view, just passes props to presentional component
 */
class PostContainer extends React.Component {
  constructor(props) {
    super(props)
    
    this.state = {
      currentPostId: 1,
      title: '',
      content: '',
    }
    
    this.fetchNext = this.fetchNext.bind(this)
  }
  
  componentDidMount() {
    this.fetchNext()
  }
  
  fetchPost(id) {
    return fetch(`//jsonplaceholder.typicode.com/posts/${id}`)
      .then(response => response.json())
      .then(({ title, body }) => {
        this.setState({
          title,
          content: body,
        })
      })
  }
  
  fetchNext() {
    this.fetchPost(this.state.currentPostId)
    this.setState({ currentPostId: this.state.currentPostId + 1 })
  }
  
  render() {
    return (
      <Post
        title={this.state.title}
        content={this.state.content}
        fetchNext={this.fetchNext}
      />
    )
  }
}

ReactDOM.render(
  <PostContainer />,
  document.getElementById('root')
)

CodePen: https://codepen.io/caderek/pen/LOrNpw?editors=0010

Oczywiście nadal możesz sobie wydzielić częśc logiki poza container component.

@Maciej Cąderek:
Przerabiam właśnie CSS-y na zgodne z BEM-em, ale zastanawiam się nad sposobem praktycznego zastosowania.

https://medium.com/@jacksleight/bem-and-sass-without-mixins-or-functions-a2f0445f504e

.block {
    $b: &;
    margin: 1em;
    &__element {
        color: red;
    }
    &--modifier {
        margin: 2em;
        #{$b} {
            &__element {
                color: blue;                    
            }    
        }
    }
}

Czy taka implementacja bedzie wygodna? Style podzielilem na moduly per block.

Cytuję siebie:

nie sklejaj w Sass nazw klas za pomoca & bo kod jest przez to nieczytelny (trzeba skakać wzrokiem) i ciężko się debuguje - bo kopiujesz sobie nazwe klasy w dev toolsach i jak to potem wyszukac w kodzie?

To co gość proponuje w artykule to imo totalnie poroniony pomysł.

@Maciej Cąderek:
Mógłbyś podrzucić jakiś sensowny materiał o tym? Samego BEM-a rozumiem, ale nie moge znalezc opisu jak dobrze wykrzystac go z SASS-em. Ewentualnie kawalek pseudokodu wystarczy.
Na pewno chcialbym zostac przy konwencji, gdzie jeden plik ze stylami odpowiada jednego blokowi. Na koniec importowac w jednym miejscu i na podstawie pliku agregujacego wszystkie importy budowac finalne style.

@Maciej Cąderek:
Mógłbyś podrzucić jakiś sensowny materiał o tym? Samego BEM-a rozumiem, ale nie moge znalezc opisu jak dobrze wykrzystac go z SASS-em. Ewentualnie kawalek pseudokodu wystarczy.
Na pewno chcialbym zostac przy konwencji, gdzie jeden plik ze stylami odpowiada jednego blokowi. Na koniec importowac w jednym miejscu i na podstawie pliku agregujacego wszystkie importy budowac finalne style.

Wedlug mnie BEM nie ma juz w ogole sensu (nawet nie wiem czy kiedykolwiek mial) zamiast budowac klasy konkatenujac jej nazwe (rozne stany itp..itd..) ja po prostu albo styluje za pomoca nazwy komponenta albo w smart component mam "glowna" klase i style po prostu pisze uzywajac zasady ze styluje zawsze zaczynajac od nazwy komponentu / glownej klasy komponenty i wtedy nie musze kombinowac z jakimis BEM jak juz uzywasz css modules albo jakakolwiek implementacje scoped css to nie masz w ogole tego problemu, no chyba ze masz jakies globable elementy w aplikacji typu footer/header + ewentualnie menu ale one nie powinno miec zadnych konfliktow z reszta komponentow.

Tym bardziej ze w react jesli sie nie myle masz bardziej podejsce "css in js" czyli jakikolwiek stan ktory potem musisz odwiercedlic w widoku sterujesz bardziej za pomoca js a nie w tradycyjny sposob

Jestem bardzo ciekaw tej dyskusji. Ostatnio troche czytałem o css-ie w komponentach czy imporcie per komponent, ale kompletnie to do mnie nie przemawiało. Miałem wrażenie, że to więcej zachodu, niż to warte. Tj. rozumiem że mając 500 różnych komponentów import wszystkiego nie ma za bardzo sensu i wtedy to jakieś rozwiązanie. Ale czy jest sens rozbijać style na N różnych plików i importować w kontenerach, kiedy suma komponentów nie przekracza 100?

@daniel_z:
Na czym wg Ciebie polega większa ilość zachodu w importowaniu stylów do komponentu vs importowaniu do zbiorczego arkusza? Ja nie widzę tutaj różnicy, natomiast podejście komponentowe ma taką przewagę nad globalnym arkuszem, że jest bardzo wygodnie zrobić w ten sposób ładną, komponentową strukturę plików:

/components
  /Article
    Article.jsx
    Article.scss

W powyższym Article jest komponentem, który stanowi blok w terminologii BEM (jeśli mam subkomponenty (elementy BEM) to nie tworze dla nich osobnych arkuszy - w BEM najmniejszą samodzielną jednostką blok i takie samo założenie stosuję jeśli chodzi o arkusze stylów).
Taka struktura jest o wiele lepsza niż wywalanie stylów do osobnego folderu i kursowanie między jedną lokacją a drugą - przy powyższej stukturze wczytywanie arkusza do komponentu jest najbardziej naturalnym rozwiązaniem. Liczba komponentów nie ma tu nic do rzeczy - aplikacje lubią się rozrastać (a nawet z małej aplikacji nie warto robić śmietnika).

@Czarny Lew
BEM ma dla mnie kilka istotnych zalet:

• jest uniwersalny - niezależny od używanego frameworka (lub jego braku),
• stanowi swoisty DSL dla frontu aplikacji, uniwersalny język niezależny od szczegółów implementacyjnych (warstwa abstrakcji),
• wymusza ładną, modularną architekturę, reużywalność na poziomie bloków

Składnia BEM (można spotkać kilka wersji) jest szczegółem implementacyjnym, przy czym jest czytelna i intuicyjna - o ile rozumie się ogólną ideę. No właśnie - i tu myślę jest największy problem - wielu programistów "używa BEMa" bez zrozumienia o co w nim chodzi - "no są bloki elementy i modyfikatory i z tego trzeba jakoś poskładać te klasy", czego efektem są potworki typu:

.some-block {
  ...
  .some-element {}
  .some-block---modifier {}
}
.some-block.some-block__some-element {}
.some-block__some-element__other-element {
  .some-block__some-element__other-element__and-one-more {}
}

gdzie ma to z BEMem niewiele wspólnego.

Ja akurat pracuje nad własnym projektem w którym mam plik scss dla każdego komponentu osobno w katalogu danego komponentu o tej samej nazwie jak komponent. Jak dla mnie jest to dość wygodne ponieważ łatwo mi jest odnaleźć dany styl użyty w tym komponencie. Ja akurat w tym projekcie stosuje bootstrapa 4 i w plikach scss nadpisuje style bootstrapa swoimi zmianami gdy chce coś zmienić. Globalne style trzymam w pliku scss imporowanym w głównym komponencie Layout w którym tez importuje style z bootstrapa.

Maciej Cąderek napisał(a):

@daniel_z:
Na czym wg Ciebie polega większa ilość zachodu w importowaniu stylów do komponentu vs importowaniu do zbiorczego arkusza? Ja nie widzę tutaj różnicy, natomiast podejście komponentowe ma taką przewagę nad globalnym arkuszem, że jest bardzo wygodnie zrobić w ten sposób ładną, komponentową strukturę plików:

/components
  /Article
    Article.jsx
    Article.scss

W powyższym Article jest komponentem, który stanowi blok w terminologii BEM (jeśli mam subkomponenty (elementy BEM) to nie tworze dla nich osobnych arkuszy - w BEM najmniejszą samodzielną jednostką blok i takie samo założenie stosuję jeśli chodzi o arkusze stylów).
Taka struktura jest o wiele lepsza niż wywalanie stylów do osobnego folderu i kursowanie między jedną lokacją a drugą - przy powyższej stukturze wczytywanie arkusza do komponentu jest najbardziej naturalnym rozwiązaniem. Liczba komponentów nie ma tu nic do rzeczy - aplikacje lubią się rozrastać (a nawet z małej aplikacji nie warto robić śmietnika).

Właściwie jak teraz o tym myśle to ma to sens. Przywykłem do struktury, gdzie wszystko jest w oddzielnych folderach (tj. style etc). Patrzac na to co teraz napisalem, style porozbijane sa i tak per komponent, wiec to w zasadzie kwestia trzymania w odpowiednich folderach i to wszystko. Dorzucajac do tego testy, mam wlasciwie kompletną modularność aplikacji, gdzie mogę przepinać wszystko pod zupełnie różne źródła danych.

Nie powiem, że przyszło to bez bólu, ale chyba w końcu się przekonałem :D.

Dzięki za wytrwałość :D

Przywykłem do struktury, gdzie wszystko jest w oddzielnych folderach (tj. style etc).

No tak jest we wszystkich tutorialach, no ale co styknie dla todo listy nieoniecznie będzie się skalować na większa aplikację (btw kupa ludzi się nie zastanawia i tak pisze nawet cos wiekszego, co jest smutne).