From 9855bfacdaef015f69e68be85f7a6d19acfc1fdb Mon Sep 17 00:00:00 2001 From: Pawel Fras Date: Mon, 1 Jul 2024 16:04:12 +0200 Subject: [PATCH 1/9] feat: CXSPA-6890 Create toggle and optimization options for propagating errors to the server --- .../server-responding-error-handler.spec.ts | 67 ++++++++++++++----- .../server-responding-error-handler.ts | 11 ++- .../optimized-ssr-engine.spec.ts | 63 +++++++++-------- .../optimized-engine/rendering-cache.spec.ts | 17 +++-- .../ssr/optimized-engine/rendering-cache.ts | 8 ++- .../ssr-optimization-options.ts | 23 +++++++ .../feature-toggles/config/feature-toggles.ts | 7 ++ .../spartacus/spartacus-features.module.ts | 1 + 8 files changed, 144 insertions(+), 53 deletions(-) diff --git a/core-libs/setup/ssr/error-handling/multi-error-handlers/server-responding-error-handler.spec.ts b/core-libs/setup/ssr/error-handling/multi-error-handlers/server-responding-error-handler.spec.ts index 3a01d1e9548..a3624a5b571 100644 --- a/core-libs/setup/ssr/error-handling/multi-error-handlers/server-responding-error-handler.spec.ts +++ b/core-libs/setup/ssr/error-handling/multi-error-handlers/server-responding-error-handler.spec.ts @@ -1,6 +1,6 @@ import { HttpErrorResponse } from '@angular/common/http'; import { TestBed } from '@angular/core/testing'; -import { OccConfig, Priority } from '@spartacus/core'; +import { FeatureConfigService, OccConfig, Priority } from '@spartacus/core'; import { CmsPageNotFoundServerErrorResponseFactory, SERVER_ERROR_RESPONSE_FACTORY, @@ -40,11 +40,13 @@ describe('ServerRespondingErrorHandler', () => { let serverRespondingErrorHandler: ServerRespondingErrorHandler; let factories: ServerErrorResponseFactory[]; let propagateServerErrorResponse: any; + let featureConfigService: FeatureConfigService; beforeEach(() => { TestBed.configureTestingModule({ providers: [ ServerRespondingErrorHandler, + FeatureConfigService, { provide: SERVER_ERROR_RESPONSE_FACTORY, useClass: CmsPageNotFoundServerErrorResponseFactory, @@ -69,6 +71,7 @@ describe('ServerRespondingErrorHandler', () => { serverRespondingErrorHandler = TestBed.inject( ServerRespondingErrorHandler ); + featureConfigService = TestBed.inject(FeatureConfigService); factories = TestBed.inject(SERVER_ERROR_RESPONSE_FACTORY); pageNotFoundFactory = factories[0] as CmsPageNotFoundServerErrorResponseFactory; @@ -86,41 +89,66 @@ describe('ServerRespondingErrorHandler', () => { jest.clearAllMocks(); }); - it('should call CmsPageNotFoundServerErrorResponseFactory', () => { - const error = new HttpErrorResponse({ - status: 404, - url: expectedUrl, + describe('when ssrErrorPropagation feature is enabled', () => { + beforeEach(() => { + jest.spyOn(featureConfigService, 'isEnabled').mockReturnValue(true); }); + it('should call CmsPageNotFoundServerErrorResponseFactory', () => { + const error = new HttpErrorResponse({ + status: 404, + url: expectedUrl, + }); - serverRespondingErrorHandler.handleError(error); + serverRespondingErrorHandler.handleError(error); + + expect(pageNotFoundFactory.create).toHaveBeenCalledWith(error); + expect(unknownServerErrorFactory.create).not.toHaveBeenCalled(); + expect(propagateServerErrorResponse as jest.Mock).toHaveBeenCalled(); + }); + + it('should call UnknownServerErrorResponseFactory as fallback', () => { + const error = { + headers: {}, + url: 'https://localhost:9002/rest/v2/unknown', + } as HttpErrorResponse; - expect(pageNotFoundFactory.create).toHaveBeenCalledWith(error); - expect(unknownServerErrorFactory.create).not.toHaveBeenCalled(); - expect(propagateServerErrorResponse as jest.Mock).toHaveBeenCalled(); + serverRespondingErrorHandler.handleError(error); + + expect(pageNotFoundFactory.create).not.toHaveBeenCalled(); + expect(unknownServerErrorFactory.create).toHaveBeenCalledWith(error); + expect(propagateServerErrorResponse as jest.Mock).toHaveBeenCalled(); + }); }); + describe('when ssrErrorPropagation feature is disabled', () => { + beforeEach(() => { + jest.spyOn(featureConfigService, 'isEnabled').mockReturnValue(false); + }); - it('should call UnknownServerErrorResponseFactory as fallback', () => { - const error = { - headers: {}, - url: 'https://localhost:9002/rest/v2/unknown', - } as HttpErrorResponse; + it('should not propagate any error', () => { + const error = { + headers: {}, + url: 'https://localhost:9002/rest/v2/unknown', + } as HttpErrorResponse; - serverRespondingErrorHandler.handleError(error); + serverRespondingErrorHandler.handleError(error); - expect(pageNotFoundFactory.create).not.toHaveBeenCalled(); - expect(unknownServerErrorFactory.create).toHaveBeenCalledWith(error); - expect(propagateServerErrorResponse as jest.Mock).toHaveBeenCalled(); + expect( + propagateServerErrorResponse as jest.Mock + ).not.toHaveBeenCalled(); + }); }); }); describe('custom factories', () => { let serverRespondingErrorHandler: ServerRespondingErrorHandler; let factories: ServerErrorResponseFactory[]; + let featureConfigService: FeatureConfigService; beforeEach(() => { TestBed.configureTestingModule({ providers: [ ServerRespondingErrorHandler, + FeatureConfigService, { provide: SERVER_ERROR_RESPONSE_FACTORY, useClass: CmsPageNotFoundServerErrorResponseFactory, @@ -147,6 +175,9 @@ describe('ServerRespondingErrorHandler', () => { ServerRespondingErrorHandler ); factories = TestBed.inject(SERVER_ERROR_RESPONSE_FACTORY); + featureConfigService = TestBed.inject(FeatureConfigService); + + jest.spyOn(featureConfigService, 'isEnabled').mockReturnValue(true); }); it('should call factory with highers priority', () => { diff --git a/core-libs/setup/ssr/error-handling/multi-error-handlers/server-responding-error-handler.ts b/core-libs/setup/ssr/error-handling/multi-error-handlers/server-responding-error-handler.ts index 3c48be51245..c37b82e34df 100644 --- a/core-libs/setup/ssr/error-handling/multi-error-handlers/server-responding-error-handler.ts +++ b/core-libs/setup/ssr/error-handling/multi-error-handlers/server-responding-error-handler.ts @@ -5,7 +5,11 @@ */ import { Injectable, inject } from '@angular/core'; -import { MultiErrorHandler, resolveApplicable } from '@spartacus/core'; +import { + FeatureConfigService, + MultiErrorHandler, + resolveApplicable, +} from '@spartacus/core'; import { SERVER_ERROR_RESPONSE_FACTORY } from '../server-error-response-factory'; import { PROPAGATE_SERVER_ERROR_RESPONSE } from '../server-error-response/propagate-server-error-response'; @@ -25,8 +29,13 @@ export class ServerRespondingErrorHandler implements MultiErrorHandler { protected propagateServerErrorResponse = inject( PROPAGATE_SERVER_ERROR_RESPONSE ); + private featureConfigService: FeatureConfigService = + inject(FeatureConfigService); handleError(error: unknown): void { + if (!this.featureConfigService.isEnabled('ssrErrorPropagation')) { + return; + } const cxServerErrorResponse = resolveApplicable( this.serverErrorResponseFactories, [error] diff --git a/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts b/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts index dc4157b4a7b..fdfd01ff55d 100644 --- a/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts +++ b/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts @@ -72,10 +72,11 @@ class TestEngineRunner { }, renderTime ?? defaultRenderTime); }; - this.optimizedSsrEngine = new OptimizedSsrEngine( - engineInstanceMock, - options - ); + this.optimizedSsrEngine = new OptimizedSsrEngine(engineInstanceMock, { + cacheStrategyResolver: + defaultSsrOptimizationOptions.cacheStrategyResolver, + ...options, + }); this.engineInstance = this.optimizedSsrEngine.engineInstance; } @@ -184,7 +185,9 @@ describe('OptimizedSsrEngine', () => { "reuseCurrentRendering": true, "debug": false, "renderingStrategyResolver": "() => ssr_optimization_options_1.RenderingStrategy.ALWAYS_SSR", - "logger": "DefaultExpressServerLogger" + "logger": "DefaultExpressServerLogger", + "cacheStrategyResolver": "(options, entry) => !(options.avoidCachingErrors === true && Boolean(entry.error))", + "avoidCachingErrors": false } } }", @@ -1407,30 +1410,32 @@ describe('OptimizedSsrEngine', () => { logger: new MockExpressServerLogger() as ExpressServerLogger, }); expect(consoleLogSpy.mock.lastCall).toMatchInlineSnapshot(` - [ - "[spartacus] SSR optimization engine initialized", - { - "options": { - "cacheSize": 3000, - "concurrency": 10, - "debug": false, - "forcedSsrTimeout": 60000, - "logger": "MockExpressServerLogger", - "maxRenderTime": 300000, - "renderingStrategyResolver": "(request) => { - if (hasExcludedUrl(request, defaultAlwaysCsrOptions.excludedUrls)) { - return ssr_optimization_options_1.RenderingStrategy.ALWAYS_CSR; - } - return shouldFallbackToCsr(request, options) - ? ssr_optimization_options_1.RenderingStrategy.ALWAYS_CSR - : ssr_optimization_options_1.RenderingStrategy.DEFAULT; - }", - "reuseCurrentRendering": true, - "timeout": 3000, - }, - }, - ] - `); + [ + "[spartacus] SSR optimization engine initialized", + { + "options": { + "avoidCachingErrors": false, + "cacheSize": 3000, + "cacheStrategyResolver": "(options, entry) => !(options.avoidCachingErrors === true && Boolean(entry.error))", + "concurrency": 10, + "debug": false, + "forcedSsrTimeout": 60000, + "logger": "MockExpressServerLogger", + "maxRenderTime": 300000, + "renderingStrategyResolver": "(request) => { + if (hasExcludedUrl(request, defaultAlwaysCsrOptions.excludedUrls)) { + return ssr_optimization_options_1.RenderingStrategy.ALWAYS_CSR; + } + return shouldFallbackToCsr(request, options) + ? ssr_optimization_options_1.RenderingStrategy.ALWAYS_CSR + : ssr_optimization_options_1.RenderingStrategy.DEFAULT; + }", + "reuseCurrentRendering": true, + "timeout": 3000, + }, + }, + ] + `); }); }); }); diff --git a/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts b/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts index 5cc6e8794e0..cd3d774a240 100644 --- a/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts +++ b/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts @@ -1,12 +1,21 @@ /// import { RenderingCache } from './rendering-cache'; +import { + SsrOptimizationOptions, + defaultSsrOptimizationOptions, +} from './ssr-optimization-options'; + +const options: SsrOptimizationOptions = { + cacheStrategyResolver: defaultSsrOptimizationOptions.cacheStrategyResolver, + avoidCachingErrors: defaultSsrOptimizationOptions.avoidCachingErrors, +}; describe('RenderingCache', () => { let renderingCache: RenderingCache; beforeEach(() => { - renderingCache = new RenderingCache({}); + renderingCache = new RenderingCache(options); }); it('should create rendering cache instance', () => { @@ -77,13 +86,13 @@ describe('RenderingCache with ttl', () => { let renderingCache: RenderingCache; beforeEach(() => { - renderingCache = new RenderingCache({ ttl: 100 }); + renderingCache = new RenderingCache({ ...options, ttl: 100 }); }); describe('get', () => { it('should return timestamp', () => { renderingCache.store('test', null, 'testHtml'); - expect(renderingCache.get('test').time).toBeTruthy(); + expect(renderingCache.get('test')?.time).toBeTruthy(); }); }); @@ -118,7 +127,7 @@ describe('RenderingCache with cacheSize', () => { let renderingCache: RenderingCache; beforeEach(() => { - renderingCache = new RenderingCache({ cacheSize: 2 }); + renderingCache = new RenderingCache({ ...options, cacheSize: 2 }); }); describe('get', () => { diff --git a/core-libs/setup/ssr/optimized-engine/rendering-cache.ts b/core-libs/setup/ssr/optimized-engine/rendering-cache.ts index 88833775be2..60aa8aec577 100644 --- a/core-libs/setup/ssr/optimized-engine/rendering-cache.ts +++ b/core-libs/setup/ssr/optimized-engine/rendering-cache.ts @@ -37,7 +37,13 @@ export class RenderingCache { this.renders.delete(this.renders.keys().next().value); } } - this.renders.set(key, entry); + // cache only if cachingStrategyResolver return true + if ( + this.options?.cacheStrategyResolver && + this.options?.cacheStrategyResolver(this.options, entry) + ) { + this.renders.set(key, entry); + } } get(key: string): RenderingEntry | undefined { diff --git a/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts b/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts index c4da1a62c6c..b379a88efa3 100644 --- a/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts +++ b/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts @@ -130,6 +130,26 @@ export interface SsrOptimizationOptions { * By default, the DefaultExpressServerLogger is used. */ logger?: ExpressServerLogger; + + /** + * Caching strategy resolver that determines the behavior of caching in RenderingCache. + * By default, the caching strategy is based on the presence of an error. + */ + cacheStrategyResolver?: ( + config: SsrOptimizationOptions, + entry: { + error?: Error | unknown; + html?: string; + } + ) => boolean; + + /** + * Avoid caching of errors. By default, this value is false. + * Caching errors is not recommended because it can lead to serving stale content. + * + * NOTE: Treat this option as a feature toggle. In a future, such an option is going to be true by default. + */ + avoidCachingErrors?: boolean; } export enum RenderingStrategy { @@ -150,4 +170,7 @@ export const defaultSsrOptimizationOptions: SsrOptimizationOptions = { defaultRenderingStrategyResolverOptions ), logger: new DefaultExpressServerLogger(), + cacheStrategyResolver: (options, entry) => + !(options.avoidCachingErrors === true && Boolean(entry.error)), + avoidCachingErrors: false, }; diff --git a/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts b/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts index f328422d3ec..8d1b4124368 100644 --- a/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts +++ b/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts @@ -43,6 +43,12 @@ export interface FeatureTogglesInterface { */ productConfiguratorAttributeTypesV2?: boolean; + /** + * Enables the propagation of errors that occur during server-side rendering (SSR) to the server. + * This allows for the errors to be properly handled, ensuring that a correct response is sent to the client instead of a malformed template. + */ + propagateErrorsToServer?: boolean; + /** * Adds asterisks to required form fields in all components existing before v2211.20 */ @@ -160,6 +166,7 @@ export const defaultFeatureToggles: Required = { pdfInvoicesSortByInvoiceDate: false, storeFrontLibCardParagraphTruncated: false, productConfiguratorAttributeTypesV2: false, + propagateErrorsToServer: false, a11yRequiredAsterisks: false, a11yQuantityOrderTabbing: false, a11yNavigationUiKeyboardControls: false, diff --git a/projects/storefrontapp/src/app/spartacus/spartacus-features.module.ts b/projects/storefrontapp/src/app/spartacus/spartacus-features.module.ts index 604a3381136..519b27a9faa 100644 --- a/projects/storefrontapp/src/app/spartacus/spartacus-features.module.ts +++ b/projects/storefrontapp/src/app/spartacus/spartacus-features.module.ts @@ -266,6 +266,7 @@ if (environment.requestedDeliveryDate) { pdfInvoicesSortByInvoiceDate: false, storeFrontLibCardParagraphTruncated: true, productConfiguratorAttributeTypesV2: true, + propagateErrorsToServer: true, a11yRequiredAsterisks: true, a11yQuantityOrderTabbing: true, a11yNavigationUiKeyboardControls: true, From 6a8bb1bf926be53e4521d993ab86de6cab8e1512 Mon Sep 17 00:00:00 2001 From: Pawel Fras Date: Fri, 5 Jul 2024 14:23:38 +0200 Subject: [PATCH 2/9] fix missed merge conflict --- .../feature-toggles/config/feature-toggles.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts b/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts index 69fb9d842ff..92cc5cc7b08 100644 --- a/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts +++ b/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts @@ -89,7 +89,7 @@ export interface FeatureTogglesInterface { * This allows for the errors to be properly handled, ensuring that a correct response is sent to the client instead of a malformed template. */ propagateErrorsToServer?: boolean; - + /** * In SSR, the following errors will be printed to logs (and additionally can also * be forwarded to ExpressJS if only the setting @@ -398,11 +398,8 @@ export const defaultFeatureToggles: Required = { pdfInvoicesSortByInvoiceDate: false, storeFrontLibCardParagraphTruncated: false, productConfiguratorAttributeTypesV2: false, -<<<<<<< HEAD propagateErrorsToServer: false, -======= ssrStrictErrorHandlingForHttpAndNgrx: false, ->>>>>>> @{-1} a11yRequiredAsterisks: false, a11yQuantityOrderTabbing: false, a11yNavigationUiKeyboardControls: false, From 55c6409f87efe8f1405b297a3ba29582384fbeb2 Mon Sep 17 00:00:00 2001 From: Pawel Fras Date: Fri, 5 Jul 2024 16:00:56 +0200 Subject: [PATCH 3/9] Trigger Build From 91def4c4f6086ebdabc6d1cb4d2af6e78e7fae34 Mon Sep 17 00:00:00 2001 From: Pawel Fras Date: Thu, 11 Jul 2024 12:51:02 +0200 Subject: [PATCH 4/9] add missing unit tests and adjust cacheStrategyResoler --- .../optimized-ssr-engine.spec.ts.snap | 33 +++++ .../rendering-cache.spec.ts.snap | 8 ++ core-libs/setup/ssr/optimized-engine/index.ts | 1 + .../optimized-ssr-engine.spec.ts | 125 ++++++++++++++++-- .../optimized-engine/rendering-cache.model.ts | 9 ++ .../optimized-engine/rendering-cache.spec.ts | 42 ++++++ .../ssr/optimized-engine/rendering-cache.ts | 8 +- .../ssr-optimization-options.ts | 8 +- nx.json | 2 +- 9 files changed, 215 insertions(+), 21 deletions(-) create mode 100644 core-libs/setup/ssr/optimized-engine/__snapshots__/optimized-ssr-engine.spec.ts.snap create mode 100644 core-libs/setup/ssr/optimized-engine/__snapshots__/rendering-cache.spec.ts.snap create mode 100644 core-libs/setup/ssr/optimized-engine/rendering-cache.model.ts diff --git a/core-libs/setup/ssr/optimized-engine/__snapshots__/optimized-ssr-engine.spec.ts.snap b/core-libs/setup/ssr/optimized-engine/__snapshots__/optimized-ssr-engine.spec.ts.snap new file mode 100644 index 00000000000..9388e54bb6a --- /dev/null +++ b/core-libs/setup/ssr/optimized-engine/__snapshots__/optimized-ssr-engine.spec.ts.snap @@ -0,0 +1,33 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`OptimizedSsrEngine avoidCachingErrors option when using default cacheStrategyResolver should cache errors if \`avoidCachingErrors\` is set to false 1`] = ` +[ + [Error: a-0], + [Error: a-0], + [Error: a-0], +] +`; + +exports[`OptimizedSsrEngine avoidCachingErrors option when using default cacheStrategyResolver should not cache errors if \`avoidCachingErrors\` is set to true 1`] = ` +[ + [Error: a-0], + [Error: a-1], + [Error: a-2], +] +`; + +exports[`OptimizedSsrEngine cacheStrategyResolver option should cache errors if \`cacheStrategyResolver\` returns true 1`] = ` +[ + [Error: a-0], + [Error: a-0], + [Error: a-0], +] +`; + +exports[`OptimizedSsrEngine cacheStrategyResolver option should not cache errors if \`cacheStrategyResolver\` returns false 1`] = ` +[ + [Error: a-0], + [Error: a-1], + [Error: a-2], +] +`; diff --git a/core-libs/setup/ssr/optimized-engine/__snapshots__/rendering-cache.spec.ts.snap b/core-libs/setup/ssr/optimized-engine/__snapshots__/rendering-cache.spec.ts.snap new file mode 100644 index 00000000000..68a5db2d0e8 --- /dev/null +++ b/core-libs/setup/ssr/optimized-engine/__snapshots__/rendering-cache.spec.ts.snap @@ -0,0 +1,8 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`RenderingCache with cacheSize RenderingCache and cacheStrategyResolver if default cacheStrategyResolver should cache if avoidCachingErrors is false 1`] = ` +{ + "err": [Error: err], + "html": "a", +} +`; diff --git a/core-libs/setup/ssr/optimized-engine/index.ts b/core-libs/setup/ssr/optimized-engine/index.ts index 18afefbeb11..cfa41066a42 100644 --- a/core-libs/setup/ssr/optimized-engine/index.ts +++ b/core-libs/setup/ssr/optimized-engine/index.ts @@ -6,6 +6,7 @@ export * from './optimized-ssr-engine'; export * from './rendering-cache'; +export * from './rendering-cache.model'; export * from './rendering-strategy-resolver'; export * from './rendering-strategy-resolver-options'; export { RequestContext, getRequestContext } from './request-context'; diff --git a/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts b/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts index 033d9f4976b..b0653aadad4 100644 --- a/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts +++ b/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts @@ -20,6 +20,7 @@ jest.mock('fs', () => ({ readFileSync: () => '', })); const consoleLogSpy = jest.spyOn(console, 'log').mockImplementation(); +const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); class MockExpressServerLogger implements Partial { log(message: string, context: ExpressServerLoggerContext): void { @@ -39,7 +40,7 @@ class MockExpressServerLogger implements Partial { */ class TestEngineRunner { /** Accumulates html output for engine runs */ - renders: string[] = []; + renders: (string | Error)[] = []; /** Accumulates response parameters for engine runs */ responseParams: object[] = []; @@ -48,7 +49,11 @@ class TestEngineRunner { optimizedSsrEngine: OptimizedSsrEngine; engineInstance: NgExpressEngineInstance; - constructor(options: SsrOptimizationOptions, renderTime?: number) { + constructor( + options: SsrOptimizationOptions, + renderTime?: number, + params?: { withError?: boolean } + ) { // mocked engine instance that will render test output in 100 milliseconds const engineInstanceMock = ( filePath: string, @@ -56,7 +61,14 @@ class TestEngineRunner { callback: SsrCallbackFn ) => { setTimeout(() => { - callback(undefined, `${filePath}-${this.renderCount++}`); + const result = `${filePath}-${this.renderCount++}`; + + if (params?.withError) { + const err = new Error(result); + callback(err, undefined); + } else { + callback(undefined, result); + } }, renderTime ?? defaultRenderTime); }; @@ -68,6 +80,14 @@ class TestEngineRunner { this.engineInstance = this.optimizedSsrEngine.engineInstance; } + /** Create engine that results with error during render */ + static withError( + options: SsrOptimizationOptions, + renderTime = defaultRenderTime + ): TestEngineRunner { + return new TestEngineRunner(options, renderTime, { withError: true }); + } + /** Run request against the engine. The result will be stored in rendering property. */ request( url: string, @@ -103,8 +123,8 @@ class TestEngineRunner { }, }; - this.engineInstance(url, optionsMock, (_, html): void => { - this.renders.push(html ?? ''); + this.engineInstance(url, optionsMock, (error, html): void => { + this.renders.push(html ?? error ?? ''); this.responseParams.push(response); }); @@ -179,7 +199,7 @@ describe('OptimizedSsrEngine', () => { "debug": false, "renderingStrategyResolver": "() => ssr_optimization_options_1.RenderingStrategy.ALWAYS_SSR", "logger": "DefaultExpressServerLogger", - "cacheStrategyResolver": "(options, entry) => !(options.avoidCachingErrors === true && Boolean(entry.error))", + "cacheStrategyResolver": "(options, entry) => !(options.avoidCachingErrors === true && Boolean(entry.err))", "avoidCachingErrors": false } } @@ -189,6 +209,34 @@ describe('OptimizedSsrEngine', () => { }); }); + describe('rendering', () => { + it('should return rendered output if no errors', fakeAsync(() => { + const originalUrl = 'a'; + const engineRunner = new TestEngineRunner({}).request('a'); + + tick(200); + expect(engineRunner.renders).toEqual(['a-0']); + expect(consoleLogSpy).toHaveBeenCalledWith( + expect.stringContaining( + `Request is resolved with the SSR rendering result (${originalUrl})` + ) + ); + })); + + it('should return error if rendering fails', fakeAsync(() => { + const originalUrl = 'a'; + const engineRunner = TestEngineRunner.withError({}).request('a'); + + tick(200); + expect(engineRunner.renders).toEqual([new Error('a-0')]); + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining( + `Request is resolved with the SSR rendering error (${originalUrl})` + ) + ); + })); + }); + describe('rendering cache', () => { it('should be initialized with default optimization options none of the custom options are provided', () => { const engineRunner = new TestEngineRunner({}); @@ -308,7 +356,6 @@ describe('OptimizedSsrEngine', () => { tick(200); expect(engineRunner.renders).toEqual(['a-0', 'a-1', 'a-2']); })); - it('should cache requests if enabled', fakeAsync(() => { const engineRunner = new TestEngineRunner({ cache: true, @@ -338,6 +385,68 @@ describe('OptimizedSsrEngine', () => { })); }); + describe('avoidCachingErrors option', () => { + describe('when using default cacheStrategyResolver', () => { + it('should not cache errors if `avoidCachingErrors` is set to true', fakeAsync(() => { + const engineRunner = TestEngineRunner.withError({ + cache: true, + avoidCachingErrors: true, + }).request('a'); + + tick(200); + engineRunner.request('a'); + tick(200); + engineRunner.request('a'); + tick(200); + expect(engineRunner.renders).toMatchSnapshot(); + })); + + it('should cache errors if `avoidCachingErrors` is set to false', fakeAsync(() => { + const engineRunner = TestEngineRunner.withError({ + cache: true, + avoidCachingErrors: false, + }).request('a'); + + tick(200); + engineRunner.request('a'); + tick(200); + engineRunner.request('a'); + tick(200); + expect(engineRunner.renders).toMatchSnapshot(); + })); + }); + }); + + describe('cacheStrategyResolver option', () => { + it('should not cache errors if `cacheStrategyResolver` returns false', fakeAsync(() => { + const engineRunner = TestEngineRunner.withError({ + cache: true, + cacheStrategyResolver: () => false, + }).request('a'); + + tick(200); + engineRunner.request('a'); + tick(200); + engineRunner.request('a'); + tick(200); + expect(engineRunner.renders).toMatchSnapshot(); + })); + + it('should cache errors if `cacheStrategyResolver` returns true', fakeAsync(() => { + const engineRunner = TestEngineRunner.withError({ + cache: true, + cacheStrategyResolver: () => true, + }).request('a'); + + tick(200); + engineRunner.request('a'); + tick(200); + engineRunner.request('a'); + tick(200); + expect(engineRunner.renders).toMatchSnapshot(); + })); + }); + describe('concurrency option', () => { it('should limit concurrency and fallback to csr', fakeAsync(() => { const engineRunner = new TestEngineRunner({ @@ -1278,7 +1387,7 @@ describe('OptimizedSsrEngine', () => { "options": { "avoidCachingErrors": false, "cacheSize": 3000, - "cacheStrategyResolver": "(options, entry) => !(options.avoidCachingErrors === true && Boolean(entry.error))", + "cacheStrategyResolver": "(options, entry) => !(options.avoidCachingErrors === true && Boolean(entry.err))", "concurrency": 10, "debug": false, "forcedSsrTimeout": 60000, diff --git a/core-libs/setup/ssr/optimized-engine/rendering-cache.model.ts b/core-libs/setup/ssr/optimized-engine/rendering-cache.model.ts new file mode 100644 index 00000000000..08d6f3e47f9 --- /dev/null +++ b/core-libs/setup/ssr/optimized-engine/rendering-cache.model.ts @@ -0,0 +1,9 @@ +/** + * Represents a rendering entry in the rendering cache. + */ +export interface RenderingEntry { + html?: any; + err?: any; + time?: number; + rendering?: boolean; +} diff --git a/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts b/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts index cd3d774a240..33fe1471d96 100644 --- a/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts +++ b/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts @@ -160,4 +160,46 @@ describe('RenderingCache with cacheSize', () => { expect(renderingCache.get('c')).toBeTruthy(); }); }); + + describe('RenderingCache and cacheStrategyResolver', () => { + let renderingCache: RenderingCache; + + describe('if default cacheStrategyResolver', () => { + it('should cache if avoidCachingErrors is false', () => { + renderingCache = new RenderingCache({ + ...options, + avoidCachingErrors: false, + }); + renderingCache.store('a', new Error('err'), 'a'); + expect(renderingCache.get('a')).toMatchSnapshot(); + }); + + it('should not cache if avoidCachingErrors is true', () => { + renderingCache = new RenderingCache({ + ...options, + avoidCachingErrors: true, + }); + renderingCache.store('a', new Error('err'), 'a'); + expect(renderingCache.get('a')).toBeFalsy(); + }); + }); + + describe('if cacheStrategyResolver is not defined', () => { + beforeEach(() => { + renderingCache = new RenderingCache({ + ...options, + cacheStrategyResolver: undefined, + }); + }); + it('should not cache a html', () => { + renderingCache.store('a', undefined, 'a'); + expect(renderingCache.get('a')).toBeFalsy(); + }); + + it('should not cache an error', () => { + renderingCache.store('a', new Error('err'), 'a'); + expect(renderingCache.get('a')).toBeFalsy(); + }); + }); + }); }); diff --git a/core-libs/setup/ssr/optimized-engine/rendering-cache.ts b/core-libs/setup/ssr/optimized-engine/rendering-cache.ts index 60aa8aec577..bb1178596a0 100644 --- a/core-libs/setup/ssr/optimized-engine/rendering-cache.ts +++ b/core-libs/setup/ssr/optimized-engine/rendering-cache.ts @@ -4,15 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { RenderingEntry } from './rendering-cache.model'; import { SsrOptimizationOptions } from './ssr-optimization-options'; -export interface RenderingEntry { - html?: any; - err?: any; - time?: number; - rendering?: boolean; -} - export class RenderingCache { protected renders = new Map(); diff --git a/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts b/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts index b379a88efa3..b2c3a66f768 100644 --- a/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts +++ b/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts @@ -6,6 +6,7 @@ import { Request } from 'express'; import { DefaultExpressServerLogger, ExpressServerLogger } from '../logger'; +import { RenderingEntry } from './rendering-cache.model'; import { defaultRenderingStrategyResolver } from './rendering-strategy-resolver'; import { defaultRenderingStrategyResolverOptions } from './rendering-strategy-resolver-options'; @@ -137,10 +138,7 @@ export interface SsrOptimizationOptions { */ cacheStrategyResolver?: ( config: SsrOptimizationOptions, - entry: { - error?: Error | unknown; - html?: string; - } + entry: Pick ) => boolean; /** @@ -171,6 +169,6 @@ export const defaultSsrOptimizationOptions: SsrOptimizationOptions = { ), logger: new DefaultExpressServerLogger(), cacheStrategyResolver: (options, entry) => - !(options.avoidCachingErrors === true && Boolean(entry.error)), + !(options.avoidCachingErrors === true && Boolean(entry.err)), avoidCachingErrors: false, }; diff --git a/nx.json b/nx.json index 738d49de045..aec8e034997 100644 --- a/nx.json +++ b/nx.json @@ -5,7 +5,7 @@ }, "targetDefaults": { "build": { - "cache": true + "cache": false }, "lint": { "cache": true From 654b23668afad5c55c8b964743c90cf2415dfc3f Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Thu, 11 Jul 2024 10:51:32 +0000 Subject: [PATCH 5/9] Add license header --- .../setup/ssr/optimized-engine/rendering-cache.model.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core-libs/setup/ssr/optimized-engine/rendering-cache.model.ts b/core-libs/setup/ssr/optimized-engine/rendering-cache.model.ts index 08d6f3e47f9..d95a53c8f0b 100644 --- a/core-libs/setup/ssr/optimized-engine/rendering-cache.model.ts +++ b/core-libs/setup/ssr/optimized-engine/rendering-cache.model.ts @@ -1,3 +1,9 @@ +/* + * SPDX-FileCopyrightText: 2024 SAP Spartacus team + * + * SPDX-License-Identifier: Apache-2.0 + */ + /** * Represents a rendering entry in the rendering cache. */ From 1d7172e64876b7507f45d71798b8d5b384ba4fa4 Mon Sep 17 00:00:00 2001 From: Pawel Fras Date: Thu, 11 Jul 2024 13:28:47 +0200 Subject: [PATCH 6/9] adjust PropagatingToServerErrorHandler to use feature toggle --- ...ropagating-to-server-error-handler.spec.ts | 27 +++++++++++++++---- .../propagating-to-server-error-handler.ts | 7 ++++- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/core-libs/setup/ssr/error-handling/multi-error-handlers/propagating-to-server-error-handler.spec.ts b/core-libs/setup/ssr/error-handling/multi-error-handlers/propagating-to-server-error-handler.spec.ts index 97bf3d981fa..85f7db95356 100644 --- a/core-libs/setup/ssr/error-handling/multi-error-handlers/propagating-to-server-error-handler.spec.ts +++ b/core-libs/setup/ssr/error-handling/multi-error-handlers/propagating-to-server-error-handler.spec.ts @@ -1,39 +1,56 @@ import { TestBed } from '@angular/core/testing'; +import { FeatureConfigService } from '@spartacus/core'; import { PROPAGATE_ERROR_TO_SERVER } from '../error-response/propagate-error-to-server'; import { PropagatingToServerErrorHandler } from './propagating-to-server-error-handler'; describe('PropagatingToServerErrorHandler', () => { describe('default factories', () => { - let serverRespondingErrorHandler: PropagatingToServerErrorHandler; + let propagatingToServerErrorHandler: PropagatingToServerErrorHandler; + let featureConfigService: FeatureConfigService; let propagateErrorResponse: any; beforeEach(() => { TestBed.configureTestingModule({ providers: [ PropagatingToServerErrorHandler, + FeatureConfigService, { provide: PROPAGATE_ERROR_TO_SERVER, useValue: jest.fn(), }, ], }); - - serverRespondingErrorHandler = TestBed.inject( + propagatingToServerErrorHandler = TestBed.inject( PropagatingToServerErrorHandler ); propagateErrorResponse = TestBed.inject(PROPAGATE_ERROR_TO_SERVER); + featureConfigService = TestBed.inject(FeatureConfigService); }); afterEach(() => { jest.clearAllMocks(); }); - it('should propagate error', () => { + it('should propagate error when propagateErrorsToServer is enabled', () => { + jest + .spyOn(featureConfigService, 'isEnabled') + .mockImplementationOnce((val) => val === 'propagateErrorsToServer'); const error = new Error('test error'); - serverRespondingErrorHandler.handleError(error); + propagatingToServerErrorHandler.handleError(error); expect(propagateErrorResponse as jest.Mock).toHaveBeenCalledWith(error); }); + + it('should not propagate error when propagateErrorsToServer is disabled', () => { + jest + .spyOn(featureConfigService, 'isEnabled') + .mockImplementationOnce((val) => !(val === 'propagateErrorsToServer')); + const error = new Error('test error'); + + propagatingToServerErrorHandler.handleError(error); + + expect(propagateErrorResponse as jest.Mock).not.toHaveBeenCalled(); + }); }); }); diff --git a/core-libs/setup/ssr/error-handling/multi-error-handlers/propagating-to-server-error-handler.ts b/core-libs/setup/ssr/error-handling/multi-error-handlers/propagating-to-server-error-handler.ts index 26e12b056d2..68a65aa0e96 100644 --- a/core-libs/setup/ssr/error-handling/multi-error-handlers/propagating-to-server-error-handler.ts +++ b/core-libs/setup/ssr/error-handling/multi-error-handlers/propagating-to-server-error-handler.ts @@ -5,7 +5,7 @@ */ import { Injectable, inject } from '@angular/core'; -import { MultiErrorHandler } from '@spartacus/core'; +import { FeatureConfigService, MultiErrorHandler } from '@spartacus/core'; import { PROPAGATE_ERROR_TO_SERVER } from '../error-response/propagate-error-to-server'; /** @@ -28,8 +28,13 @@ import { PROPAGATE_ERROR_TO_SERVER } from '../error-response/propagate-error-to- }) export class PropagatingToServerErrorHandler implements MultiErrorHandler { protected propagateErrorToServer = inject(PROPAGATE_ERROR_TO_SERVER); + private featureConfigService: FeatureConfigService = + inject(FeatureConfigService); handleError(error: unknown): void { + if (!this.featureConfigService.isEnabled('propagateErrorsToServer')) { + return; + } this.propagateErrorToServer(error); } } From 3448367c385a26e281b0e212561516e6cc937b6b Mon Sep 17 00:00:00 2001 From: Pawel Fras Date: Thu, 11 Jul 2024 13:42:52 +0200 Subject: [PATCH 7/9] revert nx.json --- nx.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nx.json b/nx.json index aec8e034997..738d49de045 100644 --- a/nx.json +++ b/nx.json @@ -5,7 +5,7 @@ }, "targetDefaults": { "build": { - "cache": false + "cache": true }, "lint": { "cache": true From ed1c2151a6a9d17f77685f32bb15d2083731332e Mon Sep 17 00:00:00 2001 From: Pawel Fras Date: Fri, 12 Jul 2024 16:33:29 +0200 Subject: [PATCH 8/9] refactor after review --- .../optimized-ssr-engine.spec.ts.snap | 33 -------- .../rendering-cache.spec.ts.snap | 8 -- .../optimized-ssr-engine.spec.ts | 80 ++++++++++++++++++- .../optimized-engine/rendering-cache.spec.ts | 27 ++++++- .../ssr/optimized-engine/rendering-cache.ts | 5 +- .../ssr-optimization-options.ts | 18 +++-- .../feature-toggles/config/feature-toggles.ts | 8 +- 7 files changed, 120 insertions(+), 59 deletions(-) delete mode 100644 core-libs/setup/ssr/optimized-engine/__snapshots__/optimized-ssr-engine.spec.ts.snap delete mode 100644 core-libs/setup/ssr/optimized-engine/__snapshots__/rendering-cache.spec.ts.snap diff --git a/core-libs/setup/ssr/optimized-engine/__snapshots__/optimized-ssr-engine.spec.ts.snap b/core-libs/setup/ssr/optimized-engine/__snapshots__/optimized-ssr-engine.spec.ts.snap deleted file mode 100644 index 9388e54bb6a..00000000000 --- a/core-libs/setup/ssr/optimized-engine/__snapshots__/optimized-ssr-engine.spec.ts.snap +++ /dev/null @@ -1,33 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`OptimizedSsrEngine avoidCachingErrors option when using default cacheStrategyResolver should cache errors if \`avoidCachingErrors\` is set to false 1`] = ` -[ - [Error: a-0], - [Error: a-0], - [Error: a-0], -] -`; - -exports[`OptimizedSsrEngine avoidCachingErrors option when using default cacheStrategyResolver should not cache errors if \`avoidCachingErrors\` is set to true 1`] = ` -[ - [Error: a-0], - [Error: a-1], - [Error: a-2], -] -`; - -exports[`OptimizedSsrEngine cacheStrategyResolver option should cache errors if \`cacheStrategyResolver\` returns true 1`] = ` -[ - [Error: a-0], - [Error: a-0], - [Error: a-0], -] -`; - -exports[`OptimizedSsrEngine cacheStrategyResolver option should not cache errors if \`cacheStrategyResolver\` returns false 1`] = ` -[ - [Error: a-0], - [Error: a-1], - [Error: a-2], -] -`; diff --git a/core-libs/setup/ssr/optimized-engine/__snapshots__/rendering-cache.spec.ts.snap b/core-libs/setup/ssr/optimized-engine/__snapshots__/rendering-cache.spec.ts.snap deleted file mode 100644 index 68a5db2d0e8..00000000000 --- a/core-libs/setup/ssr/optimized-engine/__snapshots__/rendering-cache.spec.ts.snap +++ /dev/null @@ -1,8 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`RenderingCache with cacheSize RenderingCache and cacheStrategyResolver if default cacheStrategyResolver should cache if avoidCachingErrors is false 1`] = ` -{ - "err": [Error: err], - "html": "a", -} -`; diff --git a/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts b/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts index b0653aadad4..e083466c7de 100644 --- a/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts +++ b/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts @@ -398,7 +398,11 @@ describe('OptimizedSsrEngine', () => { tick(200); engineRunner.request('a'); tick(200); - expect(engineRunner.renders).toMatchSnapshot(); + expect(engineRunner.renders).toEqual([ + new Error('a-0'), + new Error('a-1'), + new Error('a-2'), + ]); })); it('should cache errors if `avoidCachingErrors` is set to false', fakeAsync(() => { @@ -412,7 +416,39 @@ describe('OptimizedSsrEngine', () => { tick(200); engineRunner.request('a'); tick(200); - expect(engineRunner.renders).toMatchSnapshot(); + expect(engineRunner.renders).toEqual([ + new Error('a-0'), + new Error('a-0'), + new Error('a-0'), + ]); + })); + + it('should cache HTML if `avoidCachingErrors` is set to true', fakeAsync(() => { + const engineRunner = new TestEngineRunner({ + cache: true, + avoidCachingErrors: true, + }).request('a'); + + tick(200); + engineRunner.request('a'); + tick(200); + engineRunner.request('a'); + tick(200); + expect(engineRunner.renders).toEqual(['a-0', 'a-0', 'a-0']); + })); + + it('should cache HTML if `avoidCachingErrors` is set to false', fakeAsync(() => { + const engineRunner = new TestEngineRunner({ + cache: true, + avoidCachingErrors: true, + }).request('a'); + + tick(200); + engineRunner.request('a'); + tick(200); + engineRunner.request('a'); + tick(200); + expect(engineRunner.renders).toEqual(['a-0', 'a-0', 'a-0']); })); }); }); @@ -429,7 +465,11 @@ describe('OptimizedSsrEngine', () => { tick(200); engineRunner.request('a'); tick(200); - expect(engineRunner.renders).toMatchSnapshot(); + expect(engineRunner.renders).toEqual([ + new Error('a-0'), + new Error('a-1'), + new Error('a-2'), + ]); })); it('should cache errors if `cacheStrategyResolver` returns true', fakeAsync(() => { @@ -443,7 +483,39 @@ describe('OptimizedSsrEngine', () => { tick(200); engineRunner.request('a'); tick(200); - expect(engineRunner.renders).toMatchSnapshot(); + expect(engineRunner.renders).toEqual([ + new Error('a-0'), + new Error('a-0'), + new Error('a-0'), + ]); + })); + + it('should not cache HTML if `cacheStrategyResolver` returns false', fakeAsync(() => { + const engineRunner = new TestEngineRunner({ + cache: true, + cacheStrategyResolver: () => false, + }).request('a'); + + tick(200); + engineRunner.request('a'); + tick(200); + engineRunner.request('a'); + tick(200); + expect(engineRunner.renders).toEqual(['a-0', 'a-1', 'a-2']); + })); + + it('should cache HTML if `cacheStrategyResolver` returns true', fakeAsync(() => { + const engineRunner = new TestEngineRunner({ + cache: true, + cacheStrategyResolver: () => true, + }).request('a'); + + tick(200); + engineRunner.request('a'); + tick(200); + engineRunner.request('a'); + tick(200); + expect(engineRunner.renders).toEqual(['a-0', 'a-0', 'a-0']); })); }); diff --git a/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts b/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts index 33fe1471d96..a4328b5919b 100644 --- a/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts +++ b/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts @@ -165,16 +165,37 @@ describe('RenderingCache with cacheSize', () => { let renderingCache: RenderingCache; describe('if default cacheStrategyResolver', () => { - it('should cache if avoidCachingErrors is false', () => { + it('should cache HTML if avoidCachingErrors is false', () => { + renderingCache = new RenderingCache({ + ...options, + avoidCachingErrors: false, + }); + renderingCache.store('a', undefined, 'a'); + expect(renderingCache.get('a')).toEqual({ html: 'a', err: undefined }); + }); + + it('should cache HTML if avoidCachingErrors is true', () => { + renderingCache = new RenderingCache({ + ...options, + avoidCachingErrors: false, + }); + renderingCache.store('a', undefined, 'a'); + expect(renderingCache.get('a')).toEqual({ html: 'a', err: undefined }); + }); + + it('should cache errors if avoidCachingErrors is false', () => { renderingCache = new RenderingCache({ ...options, avoidCachingErrors: false, }); renderingCache.store('a', new Error('err'), 'a'); - expect(renderingCache.get('a')).toMatchSnapshot(); + expect(renderingCache.get('a')).toEqual({ + html: 'a', + err: new Error('err'), + }); }); - it('should not cache if avoidCachingErrors is true', () => { + it('should not cache errors if avoidCachingErrors is true', () => { renderingCache = new RenderingCache({ ...options, avoidCachingErrors: true, diff --git a/core-libs/setup/ssr/optimized-engine/rendering-cache.ts b/core-libs/setup/ssr/optimized-engine/rendering-cache.ts index bb1178596a0..4f1896f9bab 100644 --- a/core-libs/setup/ssr/optimized-engine/rendering-cache.ts +++ b/core-libs/setup/ssr/optimized-engine/rendering-cache.ts @@ -32,10 +32,7 @@ export class RenderingCache { } } // cache only if cachingStrategyResolver return true - if ( - this.options?.cacheStrategyResolver && - this.options?.cacheStrategyResolver(this.options, entry) - ) { + if (this.options?.cacheStrategyResolver?.(this.options, entry)) { this.renders.set(key, entry); } } diff --git a/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts b/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts index b2c3a66f768..9354f4054e4 100644 --- a/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts +++ b/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts @@ -133,8 +133,11 @@ export interface SsrOptimizationOptions { logger?: ExpressServerLogger; /** - * Caching strategy resolver that determines the behavior of caching in RenderingCache. - * By default, the caching strategy is based on the presence of an error. + * When caching is enabled, this function tell whether the given rendering result + * (html or error) should be cached. + * + * By default, all html rendering results are cached. By default, also all errors are cached + * unless the separate option `avoidCachingErrors` is enabled. */ cacheStrategyResolver?: ( config: SsrOptimizationOptions, @@ -142,10 +145,15 @@ export interface SsrOptimizationOptions { ) => boolean; /** - * Avoid caching of errors. By default, this value is false. - * Caching errors is not recommended because it can lead to serving stale content. + * Determines if rendering errors should be skipped from caching. + * + * NOTE: It's a temporary feature toggle, to be removed in the future. + * + * It's recommended to set to `true` (i.e. errors are skipped from caching), + * which will become the default behavior, when this feature toggle is removed. * - * NOTE: Treat this option as a feature toggle. In a future, such an option is going to be true by default. + * It only affects the default `cacheStrategyResolver`. + * Custom implementations of `cacheStrategyResolver` may ignore this setting. */ avoidCachingErrors?: boolean; } diff --git a/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts b/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts index 92cc5cc7b08..9494b20b7f7 100644 --- a/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts +++ b/projects/core/src/features-config/feature-toggles/config/feature-toggles.ts @@ -85,8 +85,12 @@ export interface FeatureTogglesInterface { productConfiguratorAttributeTypesV2?: boolean; /** - * Enables the propagation of errors that occur during server-side rendering (SSR) to the server. - * This allows for the errors to be properly handled, ensuring that a correct response is sent to the client instead of a malformed template. + * In a server environment (SSR or Prerendering) it propagates all errors caught in Angular app + * (in the Angular's `ErrorHandler` class) to the server layer. + * + * In SSR, such a propagation allows the server layer (e.g. ExpressJS) for handling those errors, + * e.g. sending a proper Error Page in response to the client, + * instead of a rendered HTML that is possibly malformed due to the occurred error. */ propagateErrorsToServer?: boolean; From aca1bf69fbacda0cd489cdc90291ebc3b5c925ed Mon Sep 17 00:00:00 2001 From: Pawel Fras Date: Mon, 15 Jul 2024 10:48:38 +0200 Subject: [PATCH 9/9] chore: rename 'cacheStrategyResolver' --- .../optimized-ssr-engine.spec.ts | 28 +++++++++---------- .../optimized-engine/rendering-cache.spec.ts | 11 ++++---- .../ssr/optimized-engine/rendering-cache.ts | 9 ++++-- .../ssr-optimization-options.ts | 17 ++++++----- 4 files changed, 37 insertions(+), 28 deletions(-) diff --git a/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts b/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts index e083466c7de..a8f8ddd5b6d 100644 --- a/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts +++ b/core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts @@ -73,8 +73,8 @@ class TestEngineRunner { }; this.optimizedSsrEngine = new OptimizedSsrEngine(engineInstanceMock, { - cacheStrategyResolver: - defaultSsrOptimizationOptions.cacheStrategyResolver, + shouldCacheRenderingResult: + defaultSsrOptimizationOptions.shouldCacheRenderingResult, ...options, }); this.engineInstance = this.optimizedSsrEngine.engineInstance; @@ -199,7 +199,7 @@ describe('OptimizedSsrEngine', () => { "debug": false, "renderingStrategyResolver": "() => ssr_optimization_options_1.RenderingStrategy.ALWAYS_SSR", "logger": "DefaultExpressServerLogger", - "cacheStrategyResolver": "(options, entry) => !(options.avoidCachingErrors === true && Boolean(entry.err))", + "shouldCacheRenderingResult": "({ options, entry }) => !(options.avoidCachingErrors === true && Boolean(entry.err))", "avoidCachingErrors": false } } @@ -386,7 +386,7 @@ describe('OptimizedSsrEngine', () => { }); describe('avoidCachingErrors option', () => { - describe('when using default cacheStrategyResolver', () => { + describe('when using default shouldCacheRenderingResult', () => { it('should not cache errors if `avoidCachingErrors` is set to true', fakeAsync(() => { const engineRunner = TestEngineRunner.withError({ cache: true, @@ -453,11 +453,11 @@ describe('OptimizedSsrEngine', () => { }); }); - describe('cacheStrategyResolver option', () => { - it('should not cache errors if `cacheStrategyResolver` returns false', fakeAsync(() => { + describe('shouldCacheRenderingResult option', () => { + it('should not cache errors if `shouldCacheRenderingResult` returns false', fakeAsync(() => { const engineRunner = TestEngineRunner.withError({ cache: true, - cacheStrategyResolver: () => false, + shouldCacheRenderingResult: () => false, }).request('a'); tick(200); @@ -472,10 +472,10 @@ describe('OptimizedSsrEngine', () => { ]); })); - it('should cache errors if `cacheStrategyResolver` returns true', fakeAsync(() => { + it('should cache errors if `shouldCacheRenderingResult` returns true', fakeAsync(() => { const engineRunner = TestEngineRunner.withError({ cache: true, - cacheStrategyResolver: () => true, + shouldCacheRenderingResult: () => true, }).request('a'); tick(200); @@ -490,10 +490,10 @@ describe('OptimizedSsrEngine', () => { ]); })); - it('should not cache HTML if `cacheStrategyResolver` returns false', fakeAsync(() => { + it('should not cache HTML if `shouldCacheRenderingResult` returns false', fakeAsync(() => { const engineRunner = new TestEngineRunner({ cache: true, - cacheStrategyResolver: () => false, + shouldCacheRenderingResult: () => false, }).request('a'); tick(200); @@ -504,10 +504,10 @@ describe('OptimizedSsrEngine', () => { expect(engineRunner.renders).toEqual(['a-0', 'a-1', 'a-2']); })); - it('should cache HTML if `cacheStrategyResolver` returns true', fakeAsync(() => { + it('should cache HTML if `shouldCacheRenderingResult` returns true', fakeAsync(() => { const engineRunner = new TestEngineRunner({ cache: true, - cacheStrategyResolver: () => true, + shouldCacheRenderingResult: () => true, }).request('a'); tick(200); @@ -1459,7 +1459,6 @@ describe('OptimizedSsrEngine', () => { "options": { "avoidCachingErrors": false, "cacheSize": 3000, - "cacheStrategyResolver": "(options, entry) => !(options.avoidCachingErrors === true && Boolean(entry.err))", "concurrency": 10, "debug": false, "forcedSsrTimeout": 60000, @@ -1474,6 +1473,7 @@ describe('OptimizedSsrEngine', () => { : ssr_optimization_options_1.RenderingStrategy.DEFAULT; }", "reuseCurrentRendering": true, + "shouldCacheRenderingResult": "({ options, entry }) => !(options.avoidCachingErrors === true && Boolean(entry.err))", "timeout": 3000, }, }, diff --git a/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts b/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts index a4328b5919b..cd23fe9b914 100644 --- a/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts +++ b/core-libs/setup/ssr/optimized-engine/rendering-cache.spec.ts @@ -7,7 +7,8 @@ import { } from './ssr-optimization-options'; const options: SsrOptimizationOptions = { - cacheStrategyResolver: defaultSsrOptimizationOptions.cacheStrategyResolver, + shouldCacheRenderingResult: + defaultSsrOptimizationOptions.shouldCacheRenderingResult, avoidCachingErrors: defaultSsrOptimizationOptions.avoidCachingErrors, }; @@ -161,10 +162,10 @@ describe('RenderingCache with cacheSize', () => { }); }); - describe('RenderingCache and cacheStrategyResolver', () => { + describe('RenderingCache and shouldCacheRenderingResult', () => { let renderingCache: RenderingCache; - describe('if default cacheStrategyResolver', () => { + describe('if default shouldCacheRenderingResult', () => { it('should cache HTML if avoidCachingErrors is false', () => { renderingCache = new RenderingCache({ ...options, @@ -205,11 +206,11 @@ describe('RenderingCache with cacheSize', () => { }); }); - describe('if cacheStrategyResolver is not defined', () => { + describe('if shouldCacheRenderingResult is not defined', () => { beforeEach(() => { renderingCache = new RenderingCache({ ...options, - cacheStrategyResolver: undefined, + shouldCacheRenderingResult: undefined, }); }); it('should not cache a html', () => { diff --git a/core-libs/setup/ssr/optimized-engine/rendering-cache.ts b/core-libs/setup/ssr/optimized-engine/rendering-cache.ts index 4f1896f9bab..f7700687fb7 100644 --- a/core-libs/setup/ssr/optimized-engine/rendering-cache.ts +++ b/core-libs/setup/ssr/optimized-engine/rendering-cache.ts @@ -31,8 +31,13 @@ export class RenderingCache { this.renders.delete(this.renders.keys().next().value); } } - // cache only if cachingStrategyResolver return true - if (this.options?.cacheStrategyResolver?.(this.options, entry)) { + // cache only if shouldCacheRenderingResult return true + if ( + this.options?.shouldCacheRenderingResult?.({ + options: this.options, + entry, + }) + ) { this.renders.set(key, entry); } } diff --git a/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts b/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts index 9354f4054e4..98b97c9952d 100644 --- a/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts +++ b/core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts @@ -139,10 +139,13 @@ export interface SsrOptimizationOptions { * By default, all html rendering results are cached. By default, also all errors are cached * unless the separate option `avoidCachingErrors` is enabled. */ - cacheStrategyResolver?: ( - config: SsrOptimizationOptions, - entry: Pick - ) => boolean; + shouldCacheRenderingResult?: ({ + options, + entry, + }: { + options: SsrOptimizationOptions; + entry: Pick; + }) => boolean; /** * Determines if rendering errors should be skipped from caching. @@ -152,8 +155,8 @@ export interface SsrOptimizationOptions { * It's recommended to set to `true` (i.e. errors are skipped from caching), * which will become the default behavior, when this feature toggle is removed. * - * It only affects the default `cacheStrategyResolver`. - * Custom implementations of `cacheStrategyResolver` may ignore this setting. + * It only affects the default `shouldCacheRenderingResult`. + * Custom implementations of `shouldCacheRenderingResult` may ignore this setting. */ avoidCachingErrors?: boolean; } @@ -176,7 +179,7 @@ export const defaultSsrOptimizationOptions: SsrOptimizationOptions = { defaultRenderingStrategyResolverOptions ), logger: new DefaultExpressServerLogger(), - cacheStrategyResolver: (options, entry) => + shouldCacheRenderingResult: ({ options, entry }) => !(options.avoidCachingErrors === true && Boolean(entry.err)), avoidCachingErrors: false, };