feat: add increment and add operations to NumberSignal (#2694)

* refactor(react-signals): base changes * refactor(react-signals): use event creators * refactor(react-signals): fix protected methods * refactor(react-signals): implement `update` method * introduce generic ValueSignal type * add tests for ValueSignal * support having arbitrary types in StateEvent * fix test * Add tests for ValueSignal * Add tests for ValueSignal conditional replace * add tests for ValueSignal.ts * add TSDocs for ValueSignal.ts * expose OperationSubscription in index.ts * more detailed tests for update function * Allow null as initial and future values * override get/set for NumberSignal * adapt signal generator plugin to assume null as default value * enable signal generator plugin to generate ValueSignal services * fix tests * rollback changes for accepting null in ValueSignal * fix typo in log message * fix IT according to 0 being the client-side default for NumberSignal * feat: add increment and add operations to NumberSignal Fixes #2622 * adjust tests utils subscription to have onDisconnet --------- Co-authored-by: Vlad Rindevich <[email protected]>
vaadin · Sep 3, 2024 · 1503513 · 1503513
1 parent 18357e7
commit 1503513
Show file tree

Hide file tree

Showing 4 changed files with 272 additions and 37 deletions.
diff --git a/packages/ts/react-signals/src/Signals.ts b/packages/ts/react-signals/src/Signals.ts
@@ -1,4 +1,4 @@
-import { ValueSignal } from './ValueSignal.js';
+import { type OperationSubscription, ValueSignal } from './ValueSignal.js';
 
 /**
  * A signal that holds a number value. The underlying
@@ -23,4 +23,87 @@ import { ValueSignal } from './ValueSignal.js';
  * );
  * ```
  */
-export class NumberSignal extends ValueSignal<number> {}
+export class NumberSignal extends ValueSignal<number> {
+  /**
+   * Increments the value by the specified delta. The delta can be negative to
+   * decrease the value. If no delta is provided, the value increases by 1.
+   *
+   * This method differs from using the `++` or `+=` operators directly on the
+   * signal instance. It performs an atomic operation to prevent conflicts from
+   * concurrent changes, ensuring that other users' modifications are not
+   * accidentally overwritten.
+   *
+   * By default, if there are concurrent changes on the server, this operation
+   * will keep retrying until it succeeds. The returned `OperationSubscription`
+   * can be used to cancel the operation, if needed.
+   * To disable automatic retries, set `retryOnFailure` to `false`. Note that
+   * when `retryOnFailure` is `false`, the returned `OperationSubscription` will
+   * not be cancellable.
+   *
+   * @remarks
+   * **IMPORTANT:** please note that if the operation has already succeeded on
+   * the server, calling `cancel` will not prevent or undo the operation.The
+   * `cancel` method only stops further retry attempts if a previous attempt was
+   * rejected by the server.
+   *
+   * @param delta - The delta to increment the value by. The delta can be
+   * negative. Defaults to 1.
+   * @param retryOnFailure - Whether to retry the operation in case of a
+   * concurrent value change on the server or not. Defaults to `true`.
+   *
+   * @returns An object that can be used to cancel the operation.
+   */
+  increment(delta: number = 1, retryOnFailure: boolean = true): OperationSubscription {
+    const noopCancelSubscription = { cancel: () => {} };
+    if (delta === 0) {
+      return noopCancelSubscription;
+    }
+    if (!retryOnFailure) {
+      const expected = this.value;
+      this.replace(expected, expected + delta);
+      return noopCancelSubscription;
+    }
+    return this.update((value) => value + delta);
+  }
+
+  /**
+   * Adds the provided delta to the value. The delta can be negative to perform
+   * a subtraction.
+   * This operation does not retry in case failure due to concurrent value
+   * changes from other clients have already applied to the shared value on the
+   * server. If you want the operation to be retried until it succeeds, use the
+   * overload with retry functionality.
+   *
+   * @param delta - The delta to add to the value. The delta can be negative to
+   * perform a subtraction.
+   * @see {@link NumberSignal.add|add(delta: number, retryOnFailure: boolean)}
+   */
+  add(delta: number): void;
+
+  /**
+   * Adds the provided delta to the value. The delta can be negative to perform
+   * a subtraction.
+   * If `true` passed to the `retryOnFailure` parameter, the operation is
+   * retried in case of failures due to concurrent value changes on the server
+   * until it succeeds. Note that the returned operation subscription can be
+   * used to cancel the operation.
+   *
+   * @param delta - The delta to add to the value. The delta can be negative.
+   * @param retryOnFailure - Whether to retry the operation in case of failures
+   * due to concurrent value changes on the server or not.
+   * @returns An operation subscription that can be canceled.
+   */
+  add(delta: number, retryOnFailure: boolean): OperationSubscription;
+
+  add(delta: number, retryOnFailure?: boolean): OperationSubscription | void {
+    if (delta !== 0) {
+      if (retryOnFailure) {
+        return this.increment(delta, retryOnFailure);
+      }
+      const expected = this.value;
+      this.replace(expected, expected + delta);
+    }
+    // eslint-disable-next-line no-useless-return, consistent-return
+    return;
+  }
+}
diff --git a/packages/ts/react-signals/test/Signals.spec.tsx b/packages/ts/react-signals/test/Signals.spec.tsx
@@ -9,37 +9,32 @@ import type { StateEvent } from '../src/events.js';
 import type { ServerConnectionConfig } from '../src/FullStackSignal.js';
 import { effect } from '../src/index.js';
 import { NumberSignal } from '../src/index.js';
-import { nextFrame } from './utils.js';
+import { createSubscriptionStub, nextFrame, subscribeToSignalViaEffect } from './utils.js';
 
 use(sinonChai);
 use(chaiLike);
 
 describe('@vaadin/hilla-react-signals', () => {
   let config: ServerConnectionConfig;
+  let subscription: sinon.SinonStubbedInstance<Subscription<StateEvent<number>>>;
+  let client: sinon.SinonStubbedInstance<ConnectClient>;
+
+  function simulateReceivingSnapshot(eventId: string, value: number): void {
+    const [onNextCallback] = subscription.onNext.firstCall.args;
+    onNextCallback({ id: eventId, type: 'snapshot', value });
+  }
+
+  function simulateReceivingReject(eventId: string): void {
+    const [onNextCallback] = subscription.onNext.firstCall.args;
+    // @ts-expect-error value is not used in reject events
+    onNextCallback({ id: eventId, type: 'reject', value: 0 });
+  }
 
   beforeEach(() => {
-    const client = sinon.createStubInstance(ConnectClient);
+    client = sinon.createStubInstance(ConnectClient);
     client.call.resolves();
-
-    const subscription = sinon.spy<Subscription<StateEvent<number>>>({
-      cancel() {},
-      context() {
-        return this;
-      },
-      onComplete() {
-        return this;
-      },
-      onError() {
-        return this;
-      },
-      onNext() {
-        return this;
-      },
-      onDisconnect() {
-        return this;
-      },
-    });
     // Mock the subscribe method
+    subscription = createSubscriptionStub();
     client.subscribe.returns(subscription);
     config = { client, endpoint: 'TestEndpoint', method: 'testMethod' };
   });
@@ -83,5 +78,163 @@ describe('@vaadin/hilla-react-signals', () => {
 
       expect(results).to.be.like([undefined, 42, 43]);
     });
+
+    it('should send the correct values in replace events when increment is called', () => {
+      const numberSignal = new NumberSignal(42, config);
+      subscribeToSignalViaEffect(numberSignal);
+
+      numberSignal.increment();
+      const [, , params1] = client.call.firstCall.args;
+      expect(client.call).to.have.been.calledWithMatch('SignalsHandler', 'update', {
+        clientSignalId: numberSignal.id,
+        // @ts-expect-error params.event type has id property
+        event: { id: params1?.event.id, type: 'replace', value: 43, expected: 42 },
+      });
+      // @ts-expect-error params.event type has id property
+      simulateReceivingSnapshot(params1?.event.id, 43);
+      expect(numberSignal.value).to.equal(43);
+
+      numberSignal.increment(2);
+      const [, , params2] = client.call.secondCall.args;
+      expect(client.call).to.have.been.calledWithMatch('SignalsHandler', 'update', {
+        clientSignalId: numberSignal.id,
+        // @ts-expect-error params.event type has id property
+        event: { id: params2?.event.id, type: 'replace', value: 45, expected: 43 },
+      });
+      // @ts-expect-error params.event type has id property
+      simulateReceivingSnapshot(params2?.event.id, 45);
+      expect(numberSignal.value).to.equal(45);
+
+      numberSignal.increment(-5);
+      const [, , params3] = client.call.thirdCall.args;
+      expect(client.call).to.have.been.calledWithMatch('SignalsHandler', 'update', {
+        clientSignalId: numberSignal.id,
+        // @ts-expect-error params.event type has id property
+        event: { id: params3?.event.id, type: 'replace', value: 40, expected: 45 },
+      });
+      // @ts-expect-error params.event type has id property
+      simulateReceivingSnapshot(params3?.event.id, 40);
+      expect(numberSignal.value).to.equal(40);
+    });
+
+    it('should retry after receiving reject events when increment is called by default', () => {
+      const numberSignal = new NumberSignal(42, config);
+      subscribeToSignalViaEffect(numberSignal);
+
+      const incrementSubscription = numberSignal.increment();
+      const [, , params1] = client.call.firstCall.args;
+      expect(client.call).to.have.been.calledWithMatch('SignalsHandler', 'update', {
+        clientSignalId: numberSignal.id,
+        // @ts-expect-error params.event type has id property
+        event: { id: params1?.event.id, type: 'replace', value: 43, expected: 42 },
+      });
+
+      // @ts-expect-error params.event type has id property
+      simulateReceivingReject(params1?.event.id);
+      const [, , params2] = client.call.secondCall.args;
+      expect(client.call).to.have.been.calledWithMatch('SignalsHandler', 'update', {
+        clientSignalId: numberSignal.id,
+        // @ts-expect-error params.event type has id property
+        event: { id: params2?.event.id, type: 'replace', value: 43, expected: 42 },
+      });
+
+      setTimeout(() => incrementSubscription.cancel(), 500);
+    });
+
+    it('should not retry after receiving reject events when increment is called with true as retryOnFailure', () => {
+      const numberSignal = new NumberSignal(42, config);
+      subscribeToSignalViaEffect(numberSignal);
+
+      numberSignal.increment(1, false);
+      expect(client.call).to.have.been.calledOnce;
+      const [, , params1] = client.call.firstCall.args;
+
+      // @ts-expect-error params.event type has id property
+      simulateReceivingReject(params1?.event.id);
+      // Verify that failure has not triggered the retry:
+      expect(client.call).to.have.been.calledOnce;
+    });
+
+    it('should not send any event to the server when increment is called with zero as delta', () => {
+      const numberSignal = new NumberSignal(42, config);
+      subscribeToSignalViaEffect(numberSignal);
+
+      numberSignal.increment(0);
+      expect(client.call).not.to.have.been.called;
+    });
+
+    it('should send the correct values in replace events when add is called', () => {
+      const numberSignal = new NumberSignal(42, config);
+      subscribeToSignalViaEffect(numberSignal);
+
+      numberSignal.add(5);
+      const [, , params1] = client.call.firstCall.args;
+      expect(client.call).to.have.been.calledWithMatch('SignalsHandler', 'update', {
+        clientSignalId: numberSignal.id,
+        // @ts-expect-error params.event type has id property
+        event: { id: params1?.event.id, type: 'replace', value: 47, expected: 42 },
+      });
+      // @ts-expect-error params.event type has id property
+      simulateReceivingSnapshot(params1?.event.id, 47);
+      expect(numberSignal.value).to.equal(47);
+
+      numberSignal.increment(-10);
+      const [, , params2] = client.call.secondCall.args;
+      expect(client.call).to.have.been.calledWithMatch('SignalsHandler', 'update', {
+        clientSignalId: numberSignal.id,
+        // @ts-expect-error params.event type has id property
+        event: { id: params2?.event.id, type: 'replace', value: 37, expected: 47 },
+      });
+      // @ts-expect-error params.event type has id property
+      simulateReceivingSnapshot(params2?.event.id, 37);
+      expect(numberSignal.value).to.equal(37);
+    });
+
+    it('should retry after receiving reject events when add is called and retry on failure is true', () => {
+      const numberSignal = new NumberSignal(42, config);
+      subscribeToSignalViaEffect(numberSignal);
+
+      const incrementSubscription = numberSignal.add(-2, true);
+      expect(client.call).to.have.been.calledOnce;
+      const [, , params1] = client.call.firstCall.args;
+      expect(client.call).to.have.been.calledWithMatch('SignalsHandler', 'update', {
+        clientSignalId: numberSignal.id,
+        // @ts-expect-error params.event type has id property
+        event: { id: params1?.event.id, type: 'replace', value: 40, expected: 42 },
+      });
+
+      // @ts-expect-error params.event type has id property
+      simulateReceivingReject(params1?.event.id);
+      expect(client.call).to.have.been.calledTwice;
+      const [, , params2] = client.call.secondCall.args;
+      expect(client.call).to.have.been.calledWithMatch('SignalsHandler', 'update', {
+        clientSignalId: numberSignal.id,
+        // @ts-expect-error params.event type has id property
+        event: { id: params2?.event.id, type: 'replace', value: 40, expected: 42 },
+      });
+
+      setTimeout(() => incrementSubscription.cancel(), 500);
+    });
+
+    it('should not retry after receiving reject events when add is called and retry on failure is false', () => {
+      const numberSignal = new NumberSignal(42, config);
+      subscribeToSignalViaEffect(numberSignal);
+
+      numberSignal.add(-2);
+      expect(client.call).to.have.been.calledOnce;
+      const [, , params1] = client.call.firstCall.args;
+
+      // @ts-expect-error params.event type has id property
+      simulateReceivingReject(params1?.event.id);
+      expect(client.call).to.have.been.calledOnce;
+    });
+
+    it('should not send any event to the server when add is called with zero as delta', () => {
+      const numberSignal = new NumberSignal(42, config);
+      subscribeToSignalViaEffect(numberSignal);
+
+      numberSignal.add(0);
+      expect(client.call).not.to.have.been.called;
+    });
   });
 });
diff --git a/packages/ts/react-signals/test/ValueSignal.spec.tsx b/packages/ts/react-signals/test/ValueSignal.spec.tsx
@@ -8,7 +8,7 @@ import sinonChai from 'sinon-chai';
 import type { StateEvent } from '../src/events.js';
 import type { ServerConnectionConfig } from '../src/FullStackSignal.js';
 import { effect, ValueSignal } from '../src/index.js';
-import { createSubscriptionStub, nextFrame } from './utils.js';
+import { createSubscriptionStub, nextFrame, subscribeToSignalViaEffect } from './utils.js';
 
 use(sinonChai);
 use(chaiLike);
@@ -24,14 +24,6 @@ describe('@vaadin/hilla-react-signals', () => {
   let subscription: sinon.SinonStubbedInstance<Subscription<StateEvent<string>>>;
   let client: sinon.SinonStubbedInstance<ConnectClient>;
 
-  function subscribeToSignalViaEffect<T>(signal: ValueSignal<T>): Array<T | undefined> {
-    const results: Array<T | undefined> = [];
-    effect(() => {
-      results.push(signal.value);
-    });
-    return results;
-  }
-
   beforeEach(() => {
     client = sinon.createStubInstance(ConnectClient);
     client.call.resolves();
@@ -77,10 +69,8 @@ describe('@vaadin/hilla-react-signals', () => {
       const valueSignal = new ValueSignal<string>('foo', config);
       expect(valueSignal.value).to.equal('foo');
 
-      const results: Array<string | null> = [];
-      effect(() => {
-        results.push(valueSignal.value);
-      });
+      const results = subscribeToSignalViaEffect(valueSignal);
+
       valueSignal.value = 'bar';
       valueSignal.value += 'baz';
       valueSignal.set('qux');
@@ -202,7 +192,7 @@ describe('@vaadin/hilla-react-signals', () => {
       expect(valueSignal.value).to.equal('c');
 
       // @ts-expect-error params.event type has id property
-      onNextCallback({ id: params2?.event.id, type: 'reject', value: 'dont care' });
+      onNextCallback({ id: params2?.event.id, type: 'reject' });
       expect(client.call).to.have.been.calledThrice;
       const [, , params3] = client.call.thirdCall.args;
       expect(client.call).to.have.been.calledWithMatch('SignalsHandler', 'update', {

diff --git a/packages/ts/react-signals/test/utils.ts b/packages/ts/react-signals/test/utils.ts
@@ -1,6 +1,7 @@
 import type { Subscription } from '@vaadin/hilla-frontend';
 import sinon from 'sinon';
 import type { StateEvent } from '../src/events.js';
+import { effect, type ValueSignal } from '../src/index.js';
 
 export async function nextFrame(): Promise<void> {
   return new Promise<void>((resolve) => {
@@ -30,3 +31,11 @@ export function createSubscriptionStub<T>(): sinon.SinonStubbedInstance<Subscrip
     },
   });
 }
+
+export function subscribeToSignalViaEffect<T>(signal: ValueSignal<T>): Array<T | undefined> {
+  const results: Array<T | undefined> = [];
+  effect(() => {
+    results.push(signal.value);
+  });
+  return results;
+}