From 2fe8fe181027e7889b38b6076e16d24c9f6b91d3 Mon Sep 17 00:00:00 2001 From: Soroosh Taefi Date: Mon, 23 Dec 2024 12:28:21 +0200 Subject: [PATCH] fix: throw exception if feature flag is not enabled (#2998) * fix: throw exception if feature flag is not enabled This also removes the client-side check for the feature flag to let the UI being rendered normally, and server-side get a chance to handle the requests for subscribe or update. Fixes #2937 * remove feature flag test * refine the import * apply changes from the review --- .../signals/config/SignalsConfiguration.java | 7 ++-- .../hilla/signals/handler/SignalsHandler.java | 20 ++++++++- .../signals/handler/SignalsHandlerTest.java | 16 ++++++++ .../ts/react-signals/src/FullStackSignal.ts | 15 ------- .../ts/react-signals/test/FeatureFlag.spec.ts | 41 ------------------- .../test/FullStackSignal.spec.tsx | 2 - .../react-signals/test/NumberSignal.spec.tsx | 2 - .../react-signals/test/ValueSignal.spec.tsx | 2 - packages/ts/react-signals/test/events.spec.ts | 3 -- packages/ts/react-signals/test/setup.ts | 19 --------- 10 files changed, 39 insertions(+), 88 deletions(-) delete mode 100644 packages/ts/react-signals/test/FeatureFlag.spec.ts delete mode 100644 packages/ts/react-signals/test/setup.ts diff --git a/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/config/SignalsConfiguration.java b/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/config/SignalsConfiguration.java index 7d7493771b..c9873cd58b 100644 --- a/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/config/SignalsConfiguration.java +++ b/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/config/SignalsConfiguration.java @@ -22,6 +22,7 @@ import com.vaadin.hilla.signals.Signal; import com.vaadin.hilla.signals.core.registry.SecureSignalsRegistry; import com.vaadin.hilla.signals.handler.SignalsHandler; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -62,11 +63,11 @@ public SecureSignalsRegistry signalsRegistry() { * * @return SignalsHandler endpoint instance */ - @ConditionalOnFeatureFlag("fullstackSignals") @Bean - public SignalsHandler signalsHandler() { + public SignalsHandler signalsHandler( + @Autowired(required = false) SecureSignalsRegistry signalsRegistry) { if (signalsHandler == null) { - signalsHandler = new SignalsHandler(signalsRegistry()); + signalsHandler = new SignalsHandler(signalsRegistry); } return signalsHandler; } diff --git a/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/handler/SignalsHandler.java b/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/handler/SignalsHandler.java index b4868c896f..ccd36073be 100644 --- a/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/handler/SignalsHandler.java +++ b/packages/java/endpoint/src/main/java/com/vaadin/hilla/signals/handler/SignalsHandler.java @@ -32,9 +32,19 @@ @BrowserCallable public class SignalsHandler { + private static final String FEATURE_FLAG_ERROR_MESSAGE = """ + %n + *********************************************************************************************************************** + * The Hilla Fullstack Signals API is currently considered experimental and may change in the future. * + * To use it you need to explicitly enable it in Copilot, or by adding com.vaadin.experimental.fullstackSignals=true * + * to src/main/resources/vaadin-featureflags.properties. * + *********************************************************************************************************************** + %n""" + .stripIndent(); + private final SecureSignalsRegistry registry; - public SignalsHandler(SecureSignalsRegistry registry) { + public SignalsHandler(@Nullable SecureSignalsRegistry registry) { this.registry = registry; } @@ -53,6 +63,10 @@ public SignalsHandler(SecureSignalsRegistry registry) { public Flux subscribe(String providerEndpoint, String providerMethod, String clientSignalId, ObjectNode body, @Nullable String parentClientSignalId) { + if (registry == null) { + throw new IllegalStateException( + String.format(FEATURE_FLAG_ERROR_MESSAGE)); + } try { if (parentClientSignalId != null) { return subscribe(parentClientSignalId, clientSignalId); @@ -96,6 +110,10 @@ private Flux subscribe(String parentClientSignalId, public void update(String clientSignalId, ObjectNode event) throws EndpointInvocationException.EndpointAccessDeniedException, EndpointInvocationException.EndpointNotFoundException { + if (registry == null) { + throw new IllegalStateException( + String.format(FEATURE_FLAG_ERROR_MESSAGE)); + } var parentSignalId = ListStateEvent.extractParentSignalId(event); if (parentSignalId != null) { if (registry.get(parentSignalId) == null) { diff --git a/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/handler/SignalsHandlerTest.java b/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/handler/SignalsHandlerTest.java index f117e056a7..21266aba33 100644 --- a/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/handler/SignalsHandlerTest.java +++ b/packages/java/endpoint/src/test/java/com/vaadin/hilla/signals/handler/SignalsHandlerTest.java @@ -22,6 +22,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.when; public class SignalsHandlerTest { @@ -234,4 +235,19 @@ public void when_parentSignalIdIsNotNull_andParentSignalDoesNotExist_update_thro Mockito.verify(signalsRegistry, Mockito.never()) .get(CLIENT_SIGNAL_ID_1); } + + @Test + public void when_signalRegistryIsNull_anyInteraction_throwsException() { + signalsHandler = new SignalsHandler(null); + var exception = assertThrows(IllegalStateException.class, + () -> signalsHandler.subscribe("endpoint", "method", + CLIENT_SIGNAL_ID_1, null, null)); + assertTrue(exception.getMessage().contains( + "The Hilla Fullstack Signals API is currently considered experimental")); + + exception = assertThrows(IllegalStateException.class, + () -> signalsHandler.update(CLIENT_SIGNAL_ID_1, null)); + assertTrue(exception.getMessage().contains( + "The Hilla Fullstack Signals API is currently considered experimental")); + } } diff --git a/packages/ts/react-signals/src/FullStackSignal.ts b/packages/ts/react-signals/src/FullStackSignal.ts index 8b4abe3cb1..07c569d638 100644 --- a/packages/ts/react-signals/src/FullStackSignal.ts +++ b/packages/ts/react-signals/src/FullStackSignal.ts @@ -3,14 +3,6 @@ import { nanoid } from 'nanoid'; import { computed, signal, Signal } from './core.js'; import { createSetStateEvent, type StateEvent } from './events.js'; -interface VaadinGlobal { - Vaadin?: { - featureFlags?: { - fullstackSignals?: boolean; - }; - }; -} - const ENDPOINT = 'SignalsHandler'; /** @@ -35,13 +27,6 @@ export abstract class DependencyTrackingSignal extends Signal { #subscribeCount = -1; protected constructor(value: T | undefined, onFirstSubscribe: () => void, onLastUnsubscribe: () => void) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - if (!(globalThis as VaadinGlobal).Vaadin?.featureFlags?.fullstackSignals) { - // Remove when removing feature flag - throw new Error( - `The Hilla Fullstack Signals API is currently considered experimental and may change in the future. To use it you need to explicitly enable it in Copilot or by adding com.vaadin.experimental.fullstackSignals=true to vaadin-featureflags.properties`, - ); - } super(value); this.#onFirstSubscribe = onFirstSubscribe; this.#onLastUnsubscribe = onLastUnsubscribe; diff --git a/packages/ts/react-signals/test/FeatureFlag.spec.ts b/packages/ts/react-signals/test/FeatureFlag.spec.ts deleted file mode 100644 index 6f5cd3c68e..0000000000 --- a/packages/ts/react-signals/test/FeatureFlag.spec.ts +++ /dev/null @@ -1,41 +0,0 @@ -// eslint-disable-next-line import/no-unassigned-import -import './setup.js'; - -import { expect } from 'chai'; -import sinon from 'sinon'; -import { DependencyTrackingSignal } from '../src/FullStackSignal.js'; - -describe('@vaadin/hilla-react-signals', () => { - describe('Feature-flag Expected Error', () => { - beforeEach(() => { - window.Vaadin = { - featureFlags: { - fullstackSignals: false, - }, - }; - }); - afterEach(() => { - window.Vaadin = { - featureFlags: { - fullstackSignals: true, - }, - }; - sinon.resetHistory(); - }); - const expectedError = - 'The Hilla Fullstack Signals API is currently considered experimental and may change in the future. To use it you need to explicitly enable it in Copilot or by adding com.vaadin.experimental.fullstackSignals=true to vaadin-featureflags.properties'; - class TestSignal extends DependencyTrackingSignal { - constructor(value: T | undefined, onSubscribe: () => void, onUnsubscribe: () => void) { - super(value, onSubscribe, onUnsubscribe); - this.subscribe(() => {}); // Ignores the internal subscription. - } - } - it('should throw Error when feature-flag is not enabled', () => { - const onFirstSubscribe = sinon.stub(); - const onLastUnsubscribe = sinon.stub(); - expect(() => { - const _ = new TestSignal(undefined, onFirstSubscribe, onLastUnsubscribe); - }).to.throw(expectedError); - }); - }); -}); diff --git a/packages/ts/react-signals/test/FullStackSignal.spec.tsx b/packages/ts/react-signals/test/FullStackSignal.spec.tsx index 155860afb9..75438e8d79 100644 --- a/packages/ts/react-signals/test/FullStackSignal.spec.tsx +++ b/packages/ts/react-signals/test/FullStackSignal.spec.tsx @@ -1,6 +1,4 @@ /* eslint-disable @typescript-eslint/unbound-method */ -// eslint-disable-next-line import/no-unassigned-import -import './setup.js'; import { render } from '@testing-library/react'; import { ActionOnLostSubscription, ConnectClient, type Subscription } from '@vaadin/hilla-frontend'; diff --git a/packages/ts/react-signals/test/NumberSignal.spec.tsx b/packages/ts/react-signals/test/NumberSignal.spec.tsx index 3ba8f506b5..720d7f6dcb 100644 --- a/packages/ts/react-signals/test/NumberSignal.spec.tsx +++ b/packages/ts/react-signals/test/NumberSignal.spec.tsx @@ -1,6 +1,4 @@ /* eslint-disable @typescript-eslint/unbound-method */ -// eslint-disable-next-line import/no-unassigned-import -import './setup.js'; import { render } from '@testing-library/react'; import { ConnectClient, type Subscription } from '@vaadin/hilla-frontend'; diff --git a/packages/ts/react-signals/test/ValueSignal.spec.tsx b/packages/ts/react-signals/test/ValueSignal.spec.tsx index ab391bba10..20c2288e0b 100644 --- a/packages/ts/react-signals/test/ValueSignal.spec.tsx +++ b/packages/ts/react-signals/test/ValueSignal.spec.tsx @@ -1,6 +1,4 @@ /* eslint-disable @typescript-eslint/unbound-method */ -// eslint-disable-next-line import/no-unassigned-import -import './setup.js'; import { render } from '@testing-library/react'; import { ConnectClient, type Subscription } from '@vaadin/hilla-frontend'; diff --git a/packages/ts/react-signals/test/events.spec.ts b/packages/ts/react-signals/test/events.spec.ts index 16bac4a1ae..6b930d8ec9 100644 --- a/packages/ts/react-signals/test/events.spec.ts +++ b/packages/ts/react-signals/test/events.spec.ts @@ -1,6 +1,3 @@ -// eslint-disable-next-line import/no-unassigned-import -import './setup.js'; - import { expect } from 'chai'; import { createIncrementStateEvent, diff --git a/packages/ts/react-signals/test/setup.ts b/packages/ts/react-signals/test/setup.ts deleted file mode 100644 index a3d7fe58fb..0000000000 --- a/packages/ts/react-signals/test/setup.ts +++ /dev/null @@ -1,19 +0,0 @@ -declare global { - interface Window { - Vaadin: { - featureFlags?: { - fullstackSignals: boolean; - }; - }; - } -} - -before(() => { - window.Vaadin = { - featureFlags: { - fullstackSignals: true, - }, - }; -}); - -export {};