Feedback

[vladosko]

Had some time to look at the repo, so here are my 2 cents of feedback 😄

• state property is redundant. The BehaviorSubject that is used for the store property has a method .getValue() that lets you retrieve the state value outside of observable stream.
• When using a BehaviorSubject, it's a good idea to keep it private, and expose an observable counterpart like public data$ = dataSubject.asObservable().
• Regarding fetching the game details. In the game-details.service you can remove selectedGameId and make some changes similar to these:
  game-details.service

[...]
  getGameById$(id: string): Observable<Game> {
    if (id == null) {
      // something went wrong
      return of(null);
    } else if (this.cache[id]) {
      // game details were already requested previous
      return of(this.cache[id]);
    } else {
      return this.findGameDetails(id);
    }
  }

private findGameDetails(id: string): Observable<Game> {
    const gameDetails$ = this.findGameById(id).pipe(
      catchError((err) => {
        console.error(err);
        return of(null);
      }),
      retry(2)
    );
    const screenshots$ = this.findScreenshotsById(id).pipe(
      catchError((err) => {
        console.error(err);
        return of(null);
      }),
      retry(2)
    );

    const data$ = forkJoin([gameDetails$, screenshots$]).pipe(
      filter(
        (data): data is [Game, { image: string }[]] =>
          data[0] != null && data[1] != null
      ),
      map(([gameDetails, screenshots]) => {
        // enhance game object with screenshots
        gameDetails.screenshots = screenshots;
        this.setState({ selectedGame: gameDetails });
        return gameDetails;
      }),
      tap((game) => this.updateCache(game))
    );

    this.setState({ loading: true });
    return data$.pipe(
      filter(Boolean),
      finalize(() => this.setState({ loading: false }))
    );
  }
[...]

The next thing would be to decide the way of invalidating the cache.

And in the game-details.component you would have something like:

...
export class GameDetailsComponent {
  game$ = this.route.params.pipe(
    delay(0),
    switchMap(({ id }) => {
      return this.facade.getGameById$(id);
    })
  );
  loading$ = this.facade.loadingGame$;
  constructor(
    public facade: GameDetailsFacade,
    private route: ActivatedRoute
  ) {}
}

Basically, wherever possible, try and use streams that end up in the templates through async pipe, avoiding subscribes in the component.
Regarding the facade, you can have one file .facade.ts that would have the selectors and methods to interact with the store (as you have here), or you can have .selector.ts/.query.ts to select data from the store and .service.ts or whatever you like to call it, to update the store. In the end it's all just actions/events up, date/streams down so I don't have much to add here.

I would recommend using an existing state management solution, though :)
Some libraries are:
https://ngrx.io/
https://opensource.salesforce.com/akita/
https://github.com/ngneat/elf
https://github.com/ngneat/query (something like react-query)

This account has a lot of nice packages for angular https://github.com/ngneat

For dynamic forms: https://formly.dev/ worked well last time I tried it.

[floroz]

Thank you so much @vladosko for taking the time to review this app in depth :) !

Your feedback is very accurate and absolutely spot on.

I can see immediately how your suggestions bring improvements to the app and a much clearer data flow between the service and the view.

I've put together a PR to address the points your raised, feel free to take a peek/comment if you have the time.

• Feedback-vladimir #2

And thank you for linking those resources, will definitely check them out :)