ngxtension/ngxtension-platform

[RFC] Represent notify/refresh events with signal utility

Closed this issue · 15 comments

UPDATE: Proposed utility can be viewed in this PR (feel free to continue with feedback): #277

We can run into situations where we want to imperatively indicate that something has happened in order to trigger a re-computation of a signal or an effect. This is the type of event that might have typically been represented by Subject<void>.

NOTE: This is not intended to replace the role of a Subject generally, thanks to @brandonroberts for pointing out important context to add here: if the notifier signal were triggered three times, it doesn't necessarily mean that an effect would be triggered three times because they would (if they were called in succession) be combined into the same tick, so it would only be triggered once. This may be undesirable behaviour. The purpose of this utility is to provide the ability to trigger some thing when something happens, which is just one particular case that might have been handled by a Subject.

The core idea for this came originally from @alxhub who recommended, on the removal of mutate from signals, that a "version" signal could be used to indicate that a map has been mutated - essentially just incrementing a signal by one.

This proposal deals with the map mutation situation, but also generally any situation where you want to trigger something when something else happens. The usage would look something like this:

EDIT: Updated example usage to incorporate suggestions:

// create the notifier
myTrigger = createNotifier();

// trigger the notifier
myTrigger.notify();
  // reacting to the notifier
  myThing = computedAsync(async () => {
    this.myTrigger.listen() // signal referenced to trigger computation
    // do stuff async stuff and return
  });

  // or in an effect
  constructor(){
    effect(() => {
      this.myTrigger.listen();
      // do something
    });
  }

The implementation for this utility that I created looks like this:

import { Signal, computed, signal } from '@angular/core';

export function notify(): Signal<number> & {
  notify: () => void;
  init: Signal<boolean>;
} {
  const sourceSignal = signal(0);
  const readonlySignal = sourceSignal.asReadonly();

  Object.defineProperties(readonlySignal, {
    notify: {
      value: () => sourceSignal.update((v) => v + 1),
    },
    init: {
      value: computed(() => readonlySignal() === 0),
    },
  });

  return readonlySignal as Signal<number> & {
    notify: () => void;
    init: Signal<boolean>;
  };
}

NOTE: I have also added an init signal so that if you only want to react to the notifier being explicitly triggered (not on init as well) then you can reference the init signal and check that it is false.

@eneajaho also created a similar utility that he has been using:

function triggerer() {
  const s = signal(0);
  Object.defineProperties(readonlySignal, {
    tick: {  value: () => s.update((v) => v + 1) }
  });
  return s as Signal<number> & {tick: () => void};
}

Feel free to share any thoughts you have on how this should be implemented and also feel free to leave your thoughts on the naming, for example do you prefer something like:

  • notify or notifier that is triggered with .notify()
  • triggerer that is triggered with .tick or .trigger()
  • or something else

Hello,

I like this idea and it's something I've discussed with @eneajaho like months ago, but in a different flavor.

My only main feedback is the name of the function that constructs this mechanism, nofify() is very generic (what is notify?).

Imagine that we have to re-run a computed(async) because we want to refresh it manually (which is one of the use cases).

I'd declare it like:

const requestAbcManualRefreshNotifier = createManualNofitierSignal()

Why this name?

  1. We're creating a signal
  2. It acts as a notifier
  3. It purpose is to be manual triggered

About the init thing, that's something to be thought about more.

And I'd change it to something like:

myThing = computedAsync(async () => {
  requestAbcManualRefreshNotifier.listen();
});

Thanks Renato, the naming should definitely be considered more (for me I just needed to use it in a project and didn't put too much thought into the naming initially)

I like the idea of making it more explicit, but what about:

createNotifier()

and I also like the idea of something like a listen() method (whether it ends up being called listen or something else) which makes the role more clear within the computed/effect rather than just looking like a random function call just dangling there.

  • Very needed of this
  • computedAsync is usually triggered by props signal in this way you added one more, maybe name should be ReCompute Trigger and part of options argument
e-oz commented

Some ideas:

export function notify(): Signal<{}> & {
  notify: () => void;
  isPostInit: boolean;
} {
  const sourceSignal = signal({});
  const readonlySignal = sourceSignal.asReadonly();
  let isPostInit = false;

  Object.defineProperties(readonlySignal, {
    notify: {
      value: () => {
        sourceSignal.set({});
        isPostInit = true;
      },
    },
    isPostInit: {
      get: () => isPostInit,
    },
  });

  return readonlySignal as Signal<{}> & {
    notify: () => void;
    isPostInit: boolean;
  };
}

I just realised the init property would be unnecessary as long as a version number that starts with 0 is used. We could just do this:

effect(() => {
  this.myTrigger.listen();
  // do stuff
})

For normal cases, and if we don't want to trigger on init we could do:

effect(() => {
  if(this.myTrigger.listen()){
    // do stuff only after init
  }
})

When the version is 0 the if will fail, once it has been triggered at least once it will always be true

I just realised the init property would be unnecessary as long as a version number that starts with 0 is used. We could just do this:

effect(() => {

  this.myTrigger.listen();

  // do stuff

})

For normal cases, and if we don't want to trigger on init we could do:

effect(() => {

  if(this.myTrigger.listen()){

    // do stuff only after init

  }

})

When the version is 0 the if will fail, once it has been triggered at least once it will always be true

It can be even more simpler than that (I believe).

Could try to control if you expect the listen() to emit or not for the init, so I believe that something like this could be done:

createNotifier({
    emitImmediatelly: true / false
})

I wonder if this could be achieved since signals must always emit at least once.

e-oz commented

@joshuamorony I thought you need init for better code clarity :)

@renatoaraujoc the first value will be read anyway, just because effect() is initialized and reads its signals for the first time.

@joshuamorony I thought you need init for better code clarity :)

@renatoaraujoc the first value will be read anyway, just because effect() is initialized and reads its signals for the first time.

@e-oz sometimes you're so attached to RxJS that you forget some stuff :P

I've create an initial implementation in a draft PR incorporating the feedback here - feel free to continue suggesting any specific changes you'd like to see on the PR

how we are do it this recompute

 domain = input.required<string>();

records = computedAsync(() => this.#domainService
    .getGenericDnsRecords(this.domain(), DnsRecordType.A)
    .pipe(
      mapArray((r: DnsRecordA) => { return new AGui(r) }),
      repeat({ delay: () => this.#aStateService.refetch }),
      catchError((_err) => { this.#uiService.showErrorFlashMessage("Napaka pri pridobivanju zapisov"); return of(null) })
    ));

and I for now it is ok, but this new idea I like it very much

I like the createNotifier, notify and listen naming, pretty straightforward (much more than new Subject(), .next and subscribe at least, even though you said it wasn't to replace it I think it's kinda comparable).

One question, you said it was intended to be used inside effect/computed but could we use it anywhere still? Like in any function that we want to rerun.

Another possible surface API implementation could be createListener instead of having the listen property inside the notifier.

componentInitNotifier = createNotifier();
modalInitNotifier = createNotifier();

export class CompA {
  constructor() {
    componentInitNotifier.notify()  
  }
}

export class CompB {
  constructor() {
    componentInitNotifier.notify()  
  }

  openModal() {
    modalInitNotifier.notify();
  }
}

export class CompC {
  constructor() {
    componentInitNotifier.notify()  
  }
}

// some service

export class TrackingService {
  #trackUsefulEvents = createListener(
    [componentInitNotifier, modalInitNotifier], 
    () => {
      // perform tracking
    })
}

I think it's read better to have the deps array right from the start.

On the other hand, maybe having the listener following one each other could allow more synchronous code to be triggered.

  constructor(){
    effect(() => {
      this.myTrigger.listen();
      // do something
      this.myOtherTrigger.listen();
      // do something else
    });
  }

Will the code execute synchronously? I guess it depends on what occurs, but that could be neat.

Also, do you consider the possibility to pass a data through notify, for example componentInitNotifier.notify('CompA') that could be retrieve like so data = componentInitNotifier.listen() or something.

I've create an initial implementation in a draft PR incorporating the feedback here - feel free to continue suggesting any specific changes you'd like to see on the PR

Commented on it!

@sysmat I haven't used computedAsync this way but I'm pretty sure it would work to reference the notifier and then just return the observable, but personally if I was already using an observable anyway I'd just next a subject rather than use the notifier signal

@KevTale no the mechanism relies on the signal value changing and anything referencing that signal recomputing as a result, so would have to be in computed/effect. For the computed/effect it is essentially going to be all or nothing - if any of the notifiers are triggered then the entire thing is going to run, the only thing we can really differentiate on within the computation is the initialised/not initialised state

and there is probably more stuff you could play around with here but personally I prefer to keep this particular utility on the simple side - signal value changes -> trigger re-computation wherever referenced. I think if we do thing like notifying with specific data we are getting into dangerous territory. That's sort of what I was referring to in the little warning/disclaimer - let's say we trigger a notify with valA and then valB you might write code expecting to receive both of those "events" but you might end up only ever getting valB.

If we want to force the tick we can also do:

setTimeout(() => tick())

And have this as a method called forceNotify inside the utility.

Do you think there are any use cases that make this compelling? I do worry it might stray too far into attempting to represent events with signals (versus just the small/isolated use case of triggering a computation) - another thing to consider is that of notifying with the same data twice - again people might expect it to trigger the effect twice, but if the data hasn't actually changed then it won't run, so then we'd have to add in some other workaround for that (which is doable, but to me feels like it's getting into weird territory)