#Is it okey to have multiple effect() in constructor?

26 messages · Page 1 of 1 (latest)

hasty geyser
#

Hey, is it okey to have multiple effect() in constructor for example like this?

    effect(() => {
      if (this.authState.getUserStatus() === 'error')
        this.authService.signOut();
    });
    effect(
      () => {
        if (!this.authState.isSignedIn()) return;
        AddedCompanyNotifier.listen();
        this.companyContextState.getCompanies();
      },
      { allowSignalWrites: true }
    );
    effect(
      () => {
        if (!this.authState.isSignedIn()) return;
        if (this.companyContextState.companies().status === 'success') {
          this.localStorageCompanyContextService.getItem().subscribe((q) => {
            const companyId =
              q ??
              this.companyContextState.companies().entities[0]?.id ??
              Guid.createEmpty();
            this.companyContextState.setCompanyContextId(companyId);
          });
        }
      },
      { allowSignalWrites: true }
    );
long turtle
#

Hi,
For the third effect,
I would suggest you to convert this.localStorageCompanyContextService.getItem() to Signal cause you can potentially have data leak there, with the nested subscribe inside effect.
It is rarely good to have nested subscribe, or effect or both.

$item = toSignal(this.localStorageCompanyContextService.getItem());
  $isSuccess = computed(() => this.companyContextState.companies().status === 'success');
  $companyId = computed(() => {
    if (this.$isSuccess()) {
      return this.$item()
        ?? this.companyContextState.companies().entities[0]?.id;
    }
    return null;
  });

  constructor() {
    effect(() => {
      if (this.$companyId()) {
        this.companyContextState.setCompanyContextId(companyId);
        return;
      }
      if (this.$isSuccess$() && this.$companyId() == null) {
        Guid.createEmpty();
        return;
      }
    });
  }
hasty geyser
#

Thank you @long turtle, I have one more question, should isShowingAssignRoleDialog and isShowingAddEditRoleDialog be signals?

  public isShowingAssignRoleDialog = false;
  public isShowingAddEditRoleDialog = false;
  public selectedRoleId = signal<Guid | undefined>(undefined);

  public constructor() {
    effect(() => {
      if (this.rolesState.updateRoleStatus() === 'success') {
        this.onToggleAddEditRoleDialog();
        this.msgService.add({
          severity: 'info',
          summary: this.translate.instant('success'),
          detail: this.translate.instant('successfully-updated-role'),
        });
      } else if (this.rolesState.updateRoleStatus() === 'error') {
        this.msgService.add({
          severity: 'error',
          summary: this.translate.instant('error'),
          detail: this.translate.instant('error-while-updating-role'),
        });
      }
    });
  }

  public onToggleAssignRoleDialog(roleId: Guid | undefined = undefined): void {
    this.selectedRoleId.set(roleId);
    this.isShowingAssignRoleDialog = !this.isShowingAssignRoleDialog;
  }

  public onToggleAddEditRoleDialog(roleId: Guid | undefined = undefined): void {
    this.selectedRoleId.set(roleId);
    this.isShowingAddEditRoleDialog = !this.isShowingAddEditRoleDialog;
  }
lethal plover
#

Sure - but these effects look pretty confusing to me.

hasty geyser
#

How you would refactor this?

#

I mean this effectv

lethal plover
#

Without understanding the service code or state, etc. it's hard to say. But generally:

  • a component shouldn't be responsible for logging out when auth state changes - that's an application level / router / guard thing.
  • side effectful calls like AddedCompanyNotifier.listen(); - what is this doing and why?
  • should the .subscribe() be top-level in RxJS and use toObservable for the signal state instead?
hasty geyser
#

Let's talk about last piece of code that I've sent

#

It's not good to run toast message's in effect?

lethal plover
#

The toasts seem fine to me. I'm not sure what onToggleAddEditRoleDialog is doing there, though - that seems like a separate concern.

hasty geyser
#

its used in two places inside componet html <add-edit-role-dialog [isVisible]="isShowingAddEditRoleDialog" (hide)="onToggleAddEditRoleDialog()"
<a (click)="onToggleAddEditRoleDialog(userRole.role.id)">{{ userRole.role.name }}</a>

lethal plover
#

I think the root here might be the success/error thing - succeeding or failing seem more like events than state, so I'd look first at whether that should be in a signal at all.

hasty geyser
#

I have problem, i changed isShowingAssignRoleDialog to signal, and right now when I use onToggleAssignRoleDialog inside effect, my whole page crashes and i cant do anything with page, I dont get any console errors, any idea why it behaves like that?

 public constructor() {
    effect(
      () => {
        if (this.rolesState.updateRoleStatus() === 'success') {
          this.msgService.add({
            severity: 'info',
            summary: this.translate.instant('success'),
            detail: this.translate.instant('successfully-updated-role'),
          });
          this.onToggleAddEditRoleDialog();
        } else if (this.rolesState.updateRoleStatus() === 'error') {
          this.msgService.add({
            severity: 'error',
            summary: this.translate.instant('error'),
            detail: this.translate.instant('error-while-updating-role'),
          });
        }
      },
      { allowSignalWrites: true }
    );
  }


  public onToggleAssignRoleDialog(roleId: Guid | undefined = undefined): void {
    this.selectedRoleId.set(roleId);
    this.isShowingAssignRoleDialog.set(!this.isShowingAssignRoleDialog());
  }```
lethal plover
#

this.isShowingAssignRoleDialog.set(!this.isShowingAssignRoleDialog());
you're reading the signal in the effect + changing it, so this is an infinite loop

hasty geyser
#

That's weird because in my other component I do exactly same thing and nothing crashes

#

So I need to split it to two different methods?

  public oShowAddEditRoleDialog(roleId: Guid): void {
    this.selectedRoleId.set(roleId);
    this.isShowingAddEditRoleDialog.set(true);
  }

  public onHideAddEditRoleDialog(): void {
    this.selectedRoleId.set(undefined);
    this.isShowingAddEditRoleDialog.set(false);
  }```
#

yeah okey right now it works

#

But my code was nicer when I just had one method

hasty geyser
#

or I need to stay with this two methods

lethal plover
#

Yes, several. But again, the code is trying to tell you that the architecture is wrong. isShowingAddEditRoleDialog shouldn't need to be driven from an effect, because it wants to change in response to events (success/failure of something, which is not state)

#

this.rolesState.updateRoleStatus() probably shouldn't be a signal, but an event (Observable?) that emits success/failure.

hasty geyser
lethal plover
#

Otherwise, if you're using effects to trigger arbitrary changes in the application state, it should probably be wrapped in untracked:

effect(() => {
  const status = this.rolesState.updateRoleStatus();
  untracked(() => {
    switch (status) {
      case 'success':
        this.msgService.add({
          severity: 'info',
          summary: this.translate.instant('success'),
          detail: this.translate.instant('successfully-updated-role'),
        });
        this.onToggleAddEditRoleDialog();
        break;
      case 'error':
        this.msgService.add({
          severity: 'error',
          summary: this.translate.instant('error'),
          detail: this.translate.instant('error-while-updating-role'),
        });
      }
    }
  });
});
hasty geyser
#

Can you take a look at my code that I've sent please @lethal plover

hasty geyser