OOP Repository pattern

[marklagendijk]

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]

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

[marklagendijk]

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]

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

[NetanelBasal]

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

[EricPoul]

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]

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]

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]

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

[EricPoul]

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

[EricPoul]

Here ts infers type from the assign in the constructor so ts won't allow assigning something different from the initial type later.
https://www.typescriptlang.org/play?ts=4.8.4#code/MYGwhgzhAEAqCmEAu0DeAoaXoAcBOAlgG5hLzR7xgAmA9gHYgCe0ytlA3Ot9tMA8jwBXYEnYAKAJRpMvbEgAWBCADo2laAF5oi5SuCVS8AMpjKUrrwC+PbPmJG+hsqfbwpALh1Mc8WgDNWM3IMOWx+emQgty00aHowAFt4LwByMmRUgBpoEhAhFOgAZmgrLlkwyiQhPHpozgrS9Bt0UEgYBGQAJhleexIyevhyxojBETMpXrCdJVV1cm1dVQMqF2CLRpbeQygCendpUJnltWDYgEYABksTubOY7VTU27DThdjT1aNXc0lXpqNfqOb7rNyeby+AJDaZhMYoD7aVDxJKFdKIJDZXJgfKFEplWyVeDVWpDAE2KxAA

[EricPoul]

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

[NetanelBasal]

Maybe it will be better to strictly type it

[NetanelBasal]

Or mention it in the docs

[EricPoul]

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.

[marklagendijk]

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]

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]

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]

@marklagendijk read this thread: #433

[mikkel-ol]

I'm curious why the CLI doesn't generate more declarative code. Class fields can be assigned outside of the constructor. It's easier to read and requires fewer lines of code.

Example, generated from CLI:

export interface Test {
  id: number;
}

@Injectable({ providedIn: 'root' })
export class TestRepository {
  test$: Observable<Test[]>;

  private store;

  constructor() {
    this.store = this.createStore();
    this.test$ = this.store.pipe(selectAllEntities());
  }

  private createStore(): typeof store {
    const store = createStore({ name: 'test' }, withEntities<Test>());

    return store;
  }
}

Written more declaratively it can be as simple as:

export interface Test {
  id: number;
}

@Injectable({ providedIn: 'root' })
export class TestRepository {
  private store = createStore({ name: 'test' }, withEntities<Test>());

  test$ = this.store.pipe(selectAllEntities());
}

[thomas-w-grant]

For what it is worth, I feel it is easier to read with the initializations in the constructor. Probably due to my C++ days, but I like to have the declaration of the fields separate from the initialization. Less white noise around what fields are available when I don't also initialize them where I declare them. Makes it clearer and easier to take a quick stock of what fields are available.

But I have definitely seen an increase practice of initializing outside a constructor, so I can respect your opinion.

As far as less lines of code, can't argue with that. 😄 That is just a fact. Less lines of code is not always better though.