fix: adjust increment api and remove add from NumberSignal (#2704)

* fix: adjust increment api and remove add from NumberSignal.ts This will introduce increment event for NumberSignal so that it is processed atomically on server based on the last seen value of NumberSignal without any retries. Also, this refactors the increment method to incrementBy, and also removes the add methods that were introduced in #2694 * remove unnecessary imports and chai-like usage * formatter:format * set the value locally when incrementBy is called * address sonar warnings --------- Co-authored-by: Anton Platonov <[email protected]>
vaadin · Sep 11, 2024 · 6485d1a · 6485d1a
1 parent b5fd670
commit 6485d1a
Show file tree

Hide file tree

Showing 13 changed files with 318 additions and 359 deletions.
diff --git a/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/NumberSignal.java b/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/NumberSignal.java
@@ -1,5 +1,8 @@
 package com.vaadin.hilla.signals;
 
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import com.vaadin.hilla.signals.core.event.StateEvent;
+
 /**
  * A signal that holds a number value.
  */
@@ -24,4 +27,31 @@ public NumberSignal(Double defaultValue) {
     public NumberSignal() {
         this(0.0);
     }
+
+    /**
+     * Processes the event and updates the signal value if needed. Note that
+     * this method is not thread-safe and should be called from a synchronized
+     * context.
+     *
+     * @param event
+     *            the event to process
+     * @return <code>true</code> if the event was successfully processed and the
+     *         signal value was updated, <code>false</code> otherwise.
+     */
+    @Override
+    protected boolean processEvent(ObjectNode event) {
+        try {
+            var stateEvent = new StateEvent<>(event, Double.class);
+            if (!StateEvent.EventType.INCREMENT
+                    .equals(stateEvent.getEventType())) {
+                return super.processEvent(event);
+            }
+            Double expectedValue = getValue();
+            Double newValue = expectedValue + stateEvent.getValue();
+            return super.compareAndSet(newValue, expectedValue);
+        } catch (StateEvent.InvalidEventTypeException e) {
+            throw new UnsupportedOperationException(
+                    "Unsupported JSON: " + event, e);
+        }
+    }
 }
diff --git a/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/ValueSignal.java b/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/ValueSignal.java
@@ -145,15 +145,26 @@ private ObjectNode createStatusUpdateEvent(String eventId,
         return snapshot.toJson();
     }
 
-    private boolean processEvent(ObjectNode event) {
+    /**
+     * Processes the event and updates the signal value if needed. Note that
+     * this method is not thread-safe and should be called from a synchronized
+     * context.
+     *
+     * @param event
+     *            the event to process
+     * @return <code>true</code> if the event was successfully processed and the
+     *         signal value was updated, <code>false</code> otherwise.
+     */
+    protected boolean processEvent(ObjectNode event) {
         try {
             var stateEvent = new StateEvent<>(event, valueType);
             return switch (stateEvent.getEventType()) {
                 case SET -> {
                     this.value = stateEvent.getValue();
                     yield true;
                 }
-                case REPLACE -> compareAndSet(stateEvent);
+                case REPLACE -> compareAndSet(stateEvent.getValue(),
+                    stateEvent.getExpected());
                 default -> throw new UnsupportedOperationException(
                     "Unsupported event: " + stateEvent.getEventType());
             };
@@ -163,9 +174,21 @@ private boolean processEvent(ObjectNode event) {
         }
     }
 
-    private boolean compareAndSet(StateEvent<T> stateEvent) {
-        if (Objects.equals(this.value, stateEvent.getExpected())) {
-            this.value = stateEvent.getValue();
+    /**
+     * Compares the current value with the expected value and updates the signal
+     * value if they match. Note that this method is not thread-safe and should
+     * be called from a synchronized context.
+     *
+     * @param newValue
+     *            the new value to set
+     * @param expectedValue
+     *            the expected value
+     * @return <code>true</code> if the value was successfully updated,
+     *         <code>false</code> otherwise
+     */
+    protected boolean compareAndSet(T newValue, T expectedValue) {
+        if (Objects.equals(this.value, expectedValue)) {
+            this.value = newValue;
             return true;
         }
         return false;

diff --git a/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/core/event/StateEvent.java b/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/core/event/StateEvent.java
@@ -31,7 +31,7 @@ public static final class Field {
      * Possible types of state events.
      */
     public enum EventType {
-        SNAPSHOT, SET, REPLACE, REJECT
+        SNAPSHOT, SET, REPLACE, REJECT, INCREMENT
     }
 
     /**

diff --git a/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/NumberSignalTest.java b/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/NumberSignalTest.java
@@ -96,6 +96,23 @@ public void submit_eventWithUnknownCommand_throws() {
         assertTrue(exception.getMessage().startsWith("Unsupported JSON: "));
     }
 
+    @Test
+    public void submit_eventWithIncrementCommand_incrementsValue() {
+        NumberSignal signal = new NumberSignal(42.0);
+
+        signal.submit(createIncrementEvent("2"));
+        assertEquals(44.0, signal.getValue(), 0.0);
+
+        signal.submit(createIncrementEvent("-5.5"));
+        assertEquals(38.5, signal.getValue(), 0.0);
+    }
+
+    private ObjectNode createIncrementEvent(String value) {
+        var setEvent = new StateEvent<>(UUID.randomUUID().toString(),
+                StateEvent.EventType.INCREMENT, Double.parseDouble(value));
+        return setEvent.toJson();
+    }
+
     private ObjectNode createSetEvent(String value) {
         var setEvent = new StateEvent<>(UUID.randomUUID().toString(),
                 StateEvent.EventType.SET, Double.parseDouble(value));

diff --git a/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/core/event/StateEventTest.java b/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/core/event/StateEventTest.java
@@ -134,7 +134,7 @@ public void constructor_withJsonInvalidEventType_shouldThrowInvalidEventTypeExce
                 StateEvent.InvalidEventTypeException.class,
                 () -> new StateEvent<>(json, String.class));
 
-        String expectedMessage = "Invalid event type invalidType. Type should be either of: [SNAPSHOT, SET, REPLACE, REJECT]";
+        String expectedMessage = "Invalid event type invalidType. Type should be either of: [SNAPSHOT, SET, REPLACE, REJECT, INCREMENT]";
         String actualMessage = exception.getMessage();
 
         assertTrue(actualMessage.contains(expectedMessage));
@@ -153,7 +153,7 @@ public void constructor_withJsonMissingEventType_shouldThrowInvalidEventTypeExce
                 StateEvent.InvalidEventTypeException.class,
                 () -> new StateEvent<>(json, String.class));
 
-        String expectedMessage = "Missing event type. Type is required, and should be either of: [SNAPSHOT, SET, REPLACE, REJECT]";
+        String expectedMessage = "Missing event type. Type is required, and should be either of: [SNAPSHOT, SET, REPLACE, REJECT, INCREMENT]";
         String actualMessage = exception.getMessage();
 
         assertTrue(actualMessage.contains(expectedMessage));

diff --git a/packages/ts/react-signals/src/FullStackSignal.ts b/packages/ts/react-signals/src/FullStackSignal.ts
@@ -164,6 +164,17 @@ export abstract class FullStackSignal<T> extends DependencyTrackingSignal<T> {
     this.#paused = false;
   }
 
+  /**
+   * Sets the local value of the signal without sending any events to the server
+   * @param value - The new value.
+   * @internal
+   */
+  protected setValueLocal(value: T): void {
+    this.#paused = true;
+    this.value = value;
+    this.#paused = false;
+  }
+
   /**
    * A method to update the server with the new value.
    *

diff --git a/packages/ts/react-signals/src/NumberSignal.ts b/packages/ts/react-signals/src/NumberSignal.ts
@@ -0,0 +1,49 @@
+import { createIncrementStateEvent } from './events.js';
+import { $update } from './FullStackSignal.js';
+import { ValueSignal } from './ValueSignal.js';
+
+/**
+ * A signal that holds a number value. The underlying
+ * value of this signal is stored and updated as a
+ * shared value on the server.
+ *
+ * After obtaining the NumberSignal instance from
+ * a server-side service that returns one, the value
+ * can be updated using the `value` property,
+ * and it can be read with or without the
+ * `value` property (similar to a normal signal):
+ *
+ * @example
+ * ```tsx
+ *  const counter = CounterService.counter();
+ *
+ * return (
+ *    <Button onClick={() => counter.incrementBy(1)}>
+ *      Click count: { counter }
+ *    </Button>
+ *    <Button onClick={() => counter.value = 0}>Reset</Button>
+ * );
+ * ```
+ */
+export class NumberSignal extends ValueSignal<number> {
+  /**
+   * Increments the value by the specified delta. The delta can be negative to
+   * decrease the value.
+   *
+   * This method differs from using the `++` or `+=` operators directly on the
+   * signal value. It performs an atomic operation to prevent conflicts from
+   * concurrent changes, ensuring that other users' modifications are not
+   * accidentally overwritten.
+   *
+   * @param delta - The delta to increment the value by. The delta can be
+   * negative.
+   */
+  incrementBy(delta: number): void {
+    if (delta === 0) {
+      return;
+    }
+    this.setValueLocal(this.value + delta);
+    const event = createIncrementStateEvent(delta);
+    this[$update](event);
+  }
+}
diff --git a/packages/ts/react-signals/src/Signals.ts b/packages/ts/react-signals/src/Signals.ts
diff --git a/packages/ts/react-signals/src/events.ts b/packages/ts/react-signals/src/events.ts
@@ -46,7 +46,22 @@ export function createReplaceStateEvent<T>(expected: T, value: T): ReplaceStateE
   };
 }
 
+export type IncrementStateEvent = CreateStateEventType<number, 'increment'>;
+
+export function createIncrementStateEvent(delta: number): IncrementStateEvent {
+  return {
+    id: nanoid(),
+    type: 'increment',
+    value: delta,
+  };
+}
+
 /**
  * An object that describes the change of the signal state.
  */
-export type StateEvent<T> = RejectStateEvent | ReplaceStateEvent<T> | SetStateEvent<T> | SnapshotStateEvent<T>;
+export type StateEvent<T> =
+  | IncrementStateEvent
+  | RejectStateEvent
+  | ReplaceStateEvent<T>
+  | SetStateEvent<T>
+  | SnapshotStateEvent<T>;
diff --git a/packages/ts/react-signals/src/index.ts b/packages/ts/react-signals/src/index.ts
@@ -3,7 +3,7 @@ import './polyfills.js';
 
 // eslint-disable-next-line import/export
 export * from './core.js';
-export { NumberSignal } from './Signals.js';
+export { NumberSignal } from './NumberSignal.js';
 export { ValueSignal } from './ValueSignal.js';
 export type { OperationSubscription } from './ValueSignal.js';
 export { FullStackSignal } from './FullStackSignal.js';