Open leonelvsc opened 3 weeks ago
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
Thanks @leonelvsc this seems sensible - my understanding is that it will wrap
v
back to4294967296
and then it has the space to go all the way back up to9007199254740991
then it will wrap back to4294967296
again which is fine, the main consideration is that it starts on0
and doesn't go back to0
again because this has special meaning in terms of providing the ability to not run effects on initMaybe 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:
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
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
Would you mind adding a test case in the PR for the overflow behaviour?
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
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.
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.
We can skip that test I guess.
As analyzed in issue https://github.com/ngxtension/ngxtension-platform/issues/428#issue-2360862664