Signal notifier numeric boundaries

[leonelvsc]

As analyzed in issue #428 (comment)

[leonelvsc]

This can also be replaced by sourceSignal.set(Math.random() + 1)

[joshuamorony]

Thanks @leonelvsc this seems sensible - my understanding is that it will wrap v back to 4294967296 and then it has the space to go all the way back up to 9007199254740991 then it will wrap back to 4294967296 again which is fine, the main consideration is that it starts on 0 and doesn't go back to 0 again because this has special meaning in terms of providing the ability to not run effects on init

Maybe there is a consideration here as to whether people will use the version number in some logic, but considering this is a reasonably unlikely case in the first place maybe we can just add a note to the docs about behaviour with large numbers

[leonelvsc]

Thanks @leonelvsc this seems sensible - my understanding is that it will wrap v back to 4294967296 and then it has the space to go all the way back up to 9007199254740991 then it will wrap back to 4294967296 again which is fine, the main consideration is that it starts on 0 and doesn't go back to 0 again because this has special meaning in terms of providing the ability to not run effects on init

Maybe there is a consideration here as to whether people will use the version number in some logic, but considering this is a reasonably unlikely case in the first place maybe we can just add a note to the docs about behaviour with large numbers

I've created a simple stackblitz showing the behaviour it will never reach MAX_SAFE_INT because of the >>> 0 shift it will bounce between 1 and 4294967296:

• Signal starts with 0 on initialization
• Someone calls notify() it will keep adding until (2 ** 32 - 1 ) + 1 = 4294967296
• The next time someone calls notify() it will be 1 again because ( 4294967296 >>> 0 ) is 0 + 1 = 1

https://stackblitz.com/edit/stackblitz-starters-rwqgze?file=src%2Fmain.ts

---

So another simplier solution is to initialize signal with null then on notify set "Symbol()" it will also work, if you need numeric values another solution could be Math.random() + 1

[joshuamorony]

It doesn't specifically need to be numeric for the use cases outlined in the docs, just truthy/falsey works for the purpose of the init stuff so in that sense null/Symbol would work fine. But the version number is accessible and people could be relying on these incremental version numbers for something, so I think any change to that behaviour would be considered a breaking change

But the bit shift approach still seems fine to me (unless I'm missing something), since it would be:

• 0, 1, 2, ..., 4294967296, 1, 2...

Which still satisfies the falsey/truthy requirement and keeps things incremental + infinite and non breaking

[joshuamorony]

Would you mind adding a test case in the PR for the overflow behaviour?

[leonelvsc]

Would you mind adding a test case in the PR for the overflow behaviour?

It woud implicate the call of 2 ** 32 times to notify() if i don't modify the createNotifier signature to allow an initial value, is there any other way to test this ?

for (let i = 0; i < 2 ** 32; i++) {
  trigger.notify();
}

problem here i can't mock the internal signal because is defined as const can't patch metods and have a reference to that const signal

[joshuamorony]

Good point, we could separate out the update function, e.g:

e.g:

export function createNotifier(){}

export function notifierUpdateFn(version: number){}

But I'm not a fan of modifying the API for the sake of tests. We can probably just consider it an implementation detail and leave it untested, if the existing tests pass we're good imo. I'll leave it up to you though and other reviewers can weigh in if they want.

[leonelvsc]

Well i think is better if we leave it untested but it's just my opinion. Test with the loop would look like:

it('should bounce back to version 1 after called 2 ** 32 + 1 times', () => {
    TestBed.runInInjectionContext(() => {
      const trigger = createNotifier();

      let triggerVersion: number = 0;

      const utilFn = () => {
        trigger.notify();
        triggerVersion = trigger.listen();
      };

      for (let i = 0; i < 2 ** 32; i++) {
        trigger.notify();
      }

      utilFn();
      expect(triggerVersion).toBe(1);
    });
  });

but it takes 478 seconds to complete the test

 PASS   ngxtension  libs/ngxtension/create-notifier/src/create-notifier.spec.ts (478.04 s)
  createNotifier
    ✓ should trigger on init (20 ms)
    ✓ should allow not triggering until explicitly called (4 ms)
    ✓ should continue to trigger when additional calls are made (3 ms)
    ✓ should bounce back to version 1 after called 2 ** 32 + 1 times (476559 ms)

Test Suites: 1 passed, 1 total
Tests:       4 passed, 4 total
Snapshots:   0 total
Time:        478.586 s
Ran all test suites matching /create-notifier/i.

[eneajaho]

We can skip that test I guess.