ngneat/elf

OOP Repository pattern

marklagendijk opened this issue · 18 comments

marklagendijk commented

Currently the documentation suggests the following pattern for OOP repositories:

import { createStore, withProps, select } from '@ngneat/elf';

interface AuthProps {
  user: { id: string } | null;
}

const authStore = createStore(
  { name: 'auth' },
  withProps<AuthProps>({ user: null })
);

export class AuthRepository {
  user$ = authStore.pipe(select((state) => state.user));

  updateUser(user: AuthProps['user']) {
    authStore.update((state) => ({
      ...state,
      user,
    }));
  }
}

In many cases people using this pattern would want to use Angular root services so they add the Injectable annotation:

@Injectable({ providedIn: "root" })
export class AuthRepository {

The problem with this is that the AuthRepository now has a different lifecycle than the authStore. For applications this can be fine (because a root service is also a singleton, same as authStore which is a file-level singleton).
However, things start to become problematic during testing.

I think it might be better to promote moving the store instance to a property of the repository. Besides that we now can also properly destroy the store instance whenever the service gets destroyed.
Example:

import { createStore, withProps, select } from '@ngneat/elf';

interface AuthProps {
  user: { id: string } | null;
}

export class AuthRepository implements OnDestroy {
  private readonly store = createStore(
    { name: 'auth' },
    withProps<AuthProps>({ user: null })
  );

  user$ = authStore.pipe(select((state) => state.user));

  updateUser(user: AuthProps['user']) {
    authStore.update((state) => ({
      ...state,
      user,
    }));
  }

  ngOnDestroy(): void {
    // If you have any subscriptions for side effects, unsubscribe them here.
    this.store.destroy();
  }
}

This is nice, however, this does not work well with base classes.
In the case that we want a base class we need to get a bit more creative.

Example:

import {Directive, Injectable, OnDestroy} from "@angular/core";
import { createStore, select, Store, withProps } from "@ngneat/elf";

// Note: Directive is needed here to shut up the Angular compiler. It doesn't do any harm.
@Directive()
abstract class BaseRepository<TStore extends Store> implements OnDestroy {
  protected constructor(protected store: TStore) {}

  ngOnDestroy(): void {
    // If you have any subscriptions for side effects, unsubscribe them here.
    this.store.destroy();
  }
}

interface AuthProps {
  user: { id: string } | null;
}

function createStoreFactoryFn() {
  return createStore({ name: "auth" }, withProps<AuthProps>({ user: null }));
}

@Injectable({ providedIn: "root" })
export class AuthRepository extends BaseRepository<
  ReturnType<typeof createStoreFactoryFn>
> {
  constructor() {
    super(createStoreFactoryFn());
  }

  user$ = this.store.pipe(select((state) => state.user));

  updateUser(user: AuthProps["user"]) {
    this.store.update((state) => ({
      ...state,
      user,
    }));
  }
}

Let me know what you think!
These are just some patterns I discovered during my struggles to setup component testing for an existing application.
I know it would have helped me a lot if these patterns would have been in the documentation.
At the same time it is hard for me to estimate how widely applicable they would be.

NetanelBasal commented

Our CLI has a built-in option to generate your second example.

marklagendijk commented

Thanks for the quick response.

I just tried it out. It generated the following:

@Injectable({ providedIn: "root" })
export class TestRepository {
  private store;

  constructor() {
    this.store = this.createStore();
  }

  private createStore(): typeof store {
    const store = createStore({ name: "test" }, withActiveId());

    return store;
  }
}

Unfortunately this leaves the store property as implicit any.
This can be fixed with a minimal change to the definition of that property:

@Injectable({ providedIn: "root" })
export class TestRepository {
  private readonly store: ReturnType<typeof this.createStore>;

  constructor() {
    this.store = this.createStore();
  }

  private createStore(): typeof store {
    const store = createStore({ name: "test" }, withActiveId());

    return store;
  }
}

I find it really cool how the createStore function uses the dynamic typing capabilities of Typescript to give such extensive type information for 'free'.
It would be a pity if people miss out on that, because they don't know how to use it with certain patterns.

Btw, awesome job on this library!

marklagendijk commented

If you want I can create a PR for the docs and / or the CLI, to improve them with regards to this.

NetanelBasal commented

I am pretty sure it was fine. Weird. @EricPoul do you remember? Seems like something is missing. I am from my phone.

EricPoul commented

I've just checked it in my repo and it's not an any type. We don't need to manually type a property if it is readonly and being assigned in the constructor. I've checked it in VSC and WebStorm and it works fine. It's any only until indexing is ended.

https://www.typescriptlang.org/play?ts=4.8.4#code/MYGwhgzhAEAqCmEAu0DeAoaXoAcBOAlgG5hLzR7xgAmA9gHYgCe0ytlA3Ot9tMA8jwBXYEnYAKAJRpMvbEgAWBCADo2laAF5oi5SuCVS8AMpjKUrrwC+PbPmJG+hsqfbwpALh1Mc8WgDNWM3IMOWx+emQgty00aHowAFt4LwByMmRUgBpoEhAhFOgAZmgrLlkwyiQhPHpozgrS9CsgA

marklagendijk commented

There must be something I'm missing:

  1. I'm using the latest version of all elf packages
  2. The CLI generates without the readonly modifier on the store property.
  3. Even if I add the readonly modifier it still does not give proper auto complete info (I'm on latest Angular + Typescript).
  4. If I change it to private readonly store: ReturnType<typeof this.createStore>; I do get proper auto complete info.
EricPoul commented

Yep, I guess, because it's not marked as readonly the type is gone. After you added a readonly try to restart typescript service and wait until indexation is done.

EricPoul commented

However, I delete a readonly in my repo and the type is still correct.

EricPoul commented

Maybe it's some ts configurations? strict mode or something similar?

EricPoul commented

In the TS playground noImplicitAny do the trick. If I disable this rule then the type is gone.

NetanelBasal commented

Maybe it will be better to strictly type it

NetanelBasal commented

Or mention it in the docs

EricPoul commented

I don't mind both variants. It's private property that generates automatically so I don't think that it would bother someone. Just one thing, if it's really noImplicitAny: false then I'd encourage people not to use it😄. Still don't know what is exactly because setting noImplicitAny: false in my repo doesn't break the type.

  • 😄1
marklagendijk commented

I would prefer strongly typing it in the CLI, and also updating the docs.
It's easy to remove something, but really hard to add something you don't know exists (I only found out about ReturnType yesterday).

What about adding the this.store.destroy as an implements OnDestroy?

Also an example with using a BaseRepository might be nice for the docs.

Shall I make a PR, or would one of you prefer to do it?

EricPoul commented

The repo is created with @Injectable({ providedIn: "root" }). In this case, implements OnDestroy isn't needed.
It could be done as an argument --provideInRoot=false for cli but only for cli-ng. The problem is that cli-ng is used as a plugin and I don't know if we will be able to do so.

marklagendijk commented

I'm not entirely sure what store.destroy() does an does not do, but I would expect there would be quite a risk of issues during tests.

I have been working on setting up Cypress component testing for an existing application and the only hard part was getting the repository + store to behave properly.

NetanelBasal commented

@marklagendijk read this thread: #433