From cce3198a7d9aeaa07a253dae379d773cf625b2a8 Mon Sep 17 00:00:00 2001 From: Stanislav Mishchyshyn Date: Thu, 14 Nov 2024 18:02:10 +0200 Subject: [PATCH] perf: stop using localstorage polyfill on server side as it's contains serialization/deserialization on each call to cache +ts refactoring --- .github/workflows/ci.yml | 4 +- .gitignore | 1 + ilc/.mocharc.json | 4 +- ilc/build/webpack.js | 5 + ilc/client/Client.js | 6 +- .../TransitionBlocker.spec.js | 1 - ...ransitionManagerForceBlockRemoving.spec.js | 1 - .../configuration/IlcConfigRoot.spec.js | 1 - .../registry/BrowserCacheStorage.spec.ts | 65 +++++++++++ ilc/client/registry/BrowserCacheStorage.ts | 14 +++ ilc/client/registry/factory.js | 5 - ilc/client/registry/factory.ts | 8 ++ ...er.spec.js => DefaultCacheWrapper.spec.ts} | 107 ++++++++++-------- ...CacheWrapper.js => DefaultCacheWrapper.ts} | 100 ++++++++-------- ilc/common/MemoryCacheStorage.ts | 12 ++ ilc/common/localStorage.js | 60 ---------- ilc/common/localStorage.spec.js | 59 ---------- ilc/{server => common}/types/CacheWrapper.ts | 8 +- ilc/common/types/Context.ts | 5 + ilc/server/app.js | 21 ++-- ilc/server/app.spec.js | 3 - ilc/server/errorHandler/ErrorHandler.spec.js | 7 +- ilc/server/errorHandler/factory.js | 17 +-- ilc/server/index.js | 4 +- ilc/server/ping.js | 12 -- ilc/server/registry/Registry.js | 2 +- ilc/server/registry/factory.js | 9 -- ilc/server/registry/factory.ts | 15 +++ ilc/server/routes/pingPluginFactory.ts | 23 ++++ .../routes/renderTemplateHandlerFactory.ts | 10 +- .../routes/wildcardRequestHandlerFactory.ts | 9 +- ilc/server/tailor/factory.js | 2 +- ilc/server/types/ErrorHandler.ts | 7 ++ ilc/server/types/Registry.ts | 2 +- 34 files changed, 323 insertions(+), 286 deletions(-) create mode 100644 ilc/client/registry/BrowserCacheStorage.spec.ts create mode 100644 ilc/client/registry/BrowserCacheStorage.ts delete mode 100644 ilc/client/registry/factory.js create mode 100644 ilc/client/registry/factory.ts rename ilc/common/{CacheWrapper.spec.js => DefaultCacheWrapper.spec.ts} (75%) rename ilc/common/{CacheWrapper.js => DefaultCacheWrapper.ts} (62%) create mode 100644 ilc/common/MemoryCacheStorage.ts delete mode 100644 ilc/common/localStorage.js delete mode 100644 ilc/common/localStorage.spec.js rename ilc/{server => common}/types/CacheWrapper.ts (57%) create mode 100644 ilc/common/types/Context.ts delete mode 100644 ilc/server/ping.js delete mode 100644 ilc/server/registry/factory.js create mode 100644 ilc/server/registry/factory.ts create mode 100644 ilc/server/routes/pingPluginFactory.ts create mode 100644 ilc/server/types/ErrorHandler.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0b4444954..edd074291 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -310,7 +310,7 @@ jobs: fullCoverageDiff: true autorun: false postComment: false - oldCodeCoveragePath: ./dest-server-side-tests-artifacts/coverage/coverage-summary.json + oldCodeCoveragePath: ./dest-server-side-tests-artifacts/coverage/chrome/coverage-summary.json # remove "chrome after merge PR fix cpu newCodeCoveragePath: ./src-server-side-tests-artifacts/coverage/coverage-summary.json total_delta: 2 - name: Create coverage report for ilc/client @@ -336,7 +336,7 @@ jobs: fullCoverageDiff: true autorun: false postComment: false - oldCodeCoveragePath: ./dest-merged-registry-tests-artifacts/coverage/chrome/coverage-summary.json # remove "chrome after merge PR fix cpu + oldCodeCoveragePath: ./dest-merged-registry-tests-artifacts/coverage/coverage-summary.json newCodeCoveragePath: ./src-merged-registry-tests-artifacts/coverage/coverage-summary.json total_delta: 2 - name: Compose comment diff --git a/.gitignore b/.gitignore index 9fec547e9..6b8f1a530 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,4 @@ node_modules registry/lde/dbfiles .yalc yalc.lock +.clinic diff --git a/ilc/.mocharc.json b/ilc/.mocharc.json index 2b4a57864..91ab55de0 100644 --- a/ilc/.mocharc.json +++ b/ilc/.mocharc.json @@ -1,7 +1,7 @@ { "spec": [ - "server/**/*.spec.js", - "common/**/*.spec.js" + "server/**/*.spec.{js,ts}", + "common/**/*.spec.{js,ts}" ], "require": "ts-node/register", "timeout": 5000, diff --git a/ilc/build/webpack.js b/ilc/build/webpack.js index fdb5c57d6..41cfdd324 100644 --- a/ilc/build/webpack.js +++ b/ilc/build/webpack.js @@ -47,6 +47,11 @@ module.exports = { }, plugins: [ new DuplicateIlcPluginsWebpackPlugin(ilcPluginsPath), + /** + * This plugin is a source of deprecation warning + * Can be replaced with custom plugin like this: + * https://github.com/ckeditor/ckeditor5/pull/14678/files#diff-03f45527a47a77270743a013a8b2b6abafa6a8cb1e5583b59baf3545c74a84e8R12 + */ new WrapperPlugin({ test: /\.js$/, header: () => fs.readFileSync(path.resolve(__dirname, '../public/system.js')), diff --git a/ilc/client/Client.js b/ilc/client/Client.js index 777368bba..709b23486 100644 --- a/ilc/client/Client.js +++ b/ilc/client/Client.js @@ -4,7 +4,7 @@ import { PluginManager } from 'ilc-plugins-sdk/browser'; import { PluginsLoader } from './PluginsLoader'; import UrlProcessor from '../common/UrlProcessor'; -import { appIdToNameAndSlot, removeQueryParams, addTrailingSlashToPath } from '../common/utils'; +import { appIdToNameAndSlot, addTrailingSlashToPath } from '../common/utils'; import { setNavigationErrorHandler, addNavigationHook } from './navigationEvents/setupEvents'; @@ -20,7 +20,7 @@ import { import { triggerAppChange } from './navigationEvents'; -import registryService from './registry/factory'; +import { registryFactory } from './registry/factory'; import Router from './ClientRouter'; import initIlcState from './initIlcState'; @@ -72,7 +72,6 @@ export class Client { constructor(config) { this.#configRoot = config; - this.#registryService = registryService; const pluginsLoader = new PluginsLoader(); this.#pluginManager = new PluginManager(...pluginsLoader.load()); @@ -81,6 +80,7 @@ export class Client { this.#logger = reportingPlugin.logger; + this.#registryService = registryFactory(this.#logger); this.#errorHandlerManager = new ErrorHandlerManager(this.#logger, this.#registryService); const transitionTimeout = 60000; diff --git a/ilc/client/TransitionManager/TransitionBlocker.spec.js b/ilc/client/TransitionManager/TransitionBlocker.spec.js index 9d86c73fd..1e1b52df0 100644 --- a/ilc/client/TransitionManager/TransitionBlocker.spec.js +++ b/ilc/client/TransitionManager/TransitionBlocker.spec.js @@ -53,7 +53,6 @@ describe('TransitionBlocker', () => { it('Should set onDestroy callback', () => { transitionBlocker.destroy(); - console.log(cancelTokenSpy.callCount); expect(cancelTokenSpy.callCount).to.be.equal(1); }); diff --git a/ilc/client/TransitionManager/TransitionManagerForceBlockRemoving.spec.js b/ilc/client/TransitionManager/TransitionManagerForceBlockRemoving.spec.js index 562b7356a..fa90f4b84 100644 --- a/ilc/client/TransitionManager/TransitionManagerForceBlockRemoving.spec.js +++ b/ilc/client/TransitionManager/TransitionManagerForceBlockRemoving.spec.js @@ -103,7 +103,6 @@ describe('TransitionManager Force Block removal', () => { afterEach(() => { slots.removeSlots(); - console.log(document.body.innerHTML); clock.restore(); removePageTransactionListeners(); logger.warn.resetHistory(); diff --git a/ilc/client/configuration/IlcConfigRoot.spec.js b/ilc/client/configuration/IlcConfigRoot.spec.js index aa718d915..c91b2eca6 100644 --- a/ilc/client/configuration/IlcConfigRoot.spec.js +++ b/ilc/client/configuration/IlcConfigRoot.spec.js @@ -3,7 +3,6 @@ import { getIlcConfigRoot } from './getIlcConfigRoot'; describe('IlcConfigRoot', () => { it('IlcConfigRoot should init', () => { const configRoot = getIlcConfigRoot(); - console.log(configRoot.getConfigForApps()); expect(configRoot).to.be.an('object'); }); it('IlcConfigRoot should return singleton', () => { diff --git a/ilc/client/registry/BrowserCacheStorage.spec.ts b/ilc/client/registry/BrowserCacheStorage.spec.ts new file mode 100644 index 000000000..00685afe2 --- /dev/null +++ b/ilc/client/registry/BrowserCacheStorage.spec.ts @@ -0,0 +1,65 @@ +import { expect } from 'chai'; +import sinon from 'sinon'; +import { BrowserCacheStorage } from './BrowserCacheStorage'; + +describe('BrowserCacheStorage', () => { + let cacheStorage: BrowserCacheStorage; + let mockLocalStorage: Storage; + + const sandbox = sinon.createSandbox(); + + beforeEach(() => { + // Mock the localStorage API + mockLocalStorage = { + getItem: sandbox.stub(), + setItem: sandbox.stub(), + removeItem: sandbox.stub(), + clear: sandbox.stub(), + } as unknown as Storage; + + cacheStorage = new BrowserCacheStorage(mockLocalStorage); + }); + + afterEach(() => { + // Restore the default sandbox here + sandbox.restore(); + }); + + describe('getItem', () => { + it('should return parsed data from localStorage if key exists', () => { + const key = 'testKey'; + const value = { data: 'testData' }; + + // Mock localStorage.getItem to return stringified data + (mockLocalStorage.getItem as sinon.SinonStub).withArgs(key).returns(JSON.stringify(value)); + + const result = cacheStorage.getItem(key); + + sandbox.assert.calledWith(mockLocalStorage.getItem as sinon.SinonStub, key); + expect(result).to.deep.equal(value); + }); + + it('should return null if key does not exist in localStorage', () => { + const key = 'missingKey'; + + // Mock localStorage.getItem to return null + (mockLocalStorage.getItem as sinon.SinonStub).withArgs(key).returns(null); + + const result = cacheStorage.getItem(key); + + sandbox.assert.calledWith(mockLocalStorage.getItem as sinon.SinonStub, key); + expect(result).to.be.null; + }); + }); + + describe('setItem', () => { + it('should store stringified data in localStorage', () => { + const key = 'testKey'; + const value = { data: 'testData', cachedAt: Date.now() }; + + cacheStorage.setItem(key, value); + + sandbox.assert.calledWith(mockLocalStorage.setItem as sinon.SinonStub, key, JSON.stringify(value)); + }); + }); +}); diff --git a/ilc/client/registry/BrowserCacheStorage.ts b/ilc/client/registry/BrowserCacheStorage.ts new file mode 100644 index 000000000..1a75bd70a --- /dev/null +++ b/ilc/client/registry/BrowserCacheStorage.ts @@ -0,0 +1,14 @@ +import { CacheResult, CacheStorage } from '../../common/types/CacheWrapper'; + +export class BrowserCacheStorage implements CacheStorage { + constructor(private readonly storage: Storage) {} + + getItem(key: string): CacheResult { + const cache = this.storage.getItem(key); + return cache ? JSON.parse(cache) : null; + } + + setItem(key: string, cache: CacheResult): void { + this.storage.setItem(key, JSON.stringify(cache)); + } +} diff --git a/ilc/client/registry/factory.js b/ilc/client/registry/factory.js deleted file mode 100644 index d33731ffc..000000000 --- a/ilc/client/registry/factory.js +++ /dev/null @@ -1,5 +0,0 @@ -import Registry from './Registry'; -import CacheWrapper from '../../common/CacheWrapper'; -import localStorage from '../../common/localStorage'; - -export default new Registry(new CacheWrapper(localStorage, console)); diff --git a/ilc/client/registry/factory.ts b/ilc/client/registry/factory.ts new file mode 100644 index 000000000..d7bfb4194 --- /dev/null +++ b/ilc/client/registry/factory.ts @@ -0,0 +1,8 @@ +import type { Logger } from 'ilc-plugins-sdk'; +import type { IlcLogger } from 'ilc-plugins-sdk/browser'; +import { DefaultCacheWrapper } from '../../common/DefaultCacheWrapper'; +import { BrowserCacheStorage } from './BrowserCacheStorage'; +import Registry from './Registry'; + +export const registryFactory = (logger: IlcLogger) => + new Registry(new DefaultCacheWrapper(new BrowserCacheStorage(window.localStorage), logger as Logger)); diff --git a/ilc/common/CacheWrapper.spec.js b/ilc/common/DefaultCacheWrapper.spec.ts similarity index 75% rename from ilc/common/CacheWrapper.spec.js rename to ilc/common/DefaultCacheWrapper.spec.ts index 2b329049d..1bb113779 100644 --- a/ilc/common/CacheWrapper.spec.js +++ b/ilc/common/DefaultCacheWrapper.spec.ts @@ -1,7 +1,9 @@ +import { type ExtendedError } from '@namecheap/error-extender'; import chai from 'chai'; +import { type Logger } from 'ilc-plugins-sdk'; import sinon from 'sinon'; -import localStorage from './localStorage'; -import CacheWrapper from './CacheWrapper'; +import { DefaultCacheWrapper } from './DefaultCacheWrapper'; +import { CacheParams, CacheResult, CacheStorage } from './types/CacheWrapper'; import { TimeoutError } from './utils'; /** @@ -11,15 +13,17 @@ async function setImmediatePromise() { return new Promise((resolve) => setTimeout(resolve, 0)); } -describe('CacheWrapper', () => { - let wrappedFn, clock; +describe('DefaultCacheWrapper', () => { + let wrappedFn: (...args: any[]) => Promise>; + let clock: sinon.SinonFakeTimers; + let storageMock: CacheStorage; const fn = sinon.stub(); const createHash = sinon.stub(); - const logger = { + const loggerMock = { error: sinon.stub(), - info: function () {}, - }; + info: sinon.stub(), + } as unknown as sinon.SinonStubbedInstance; const fnError = new Error('Error message'); const fnArgs = ['firstArg', 'secondArg', 'thirdArg']; @@ -38,20 +42,24 @@ describe('CacheWrapper', () => { }; beforeEach(() => { - const cacheWrapper = new CacheWrapper(localStorage, logger); + const storageMockCache: Record> = {}; + storageMock = { + getItem: (key) => storageMockCache[key] ?? null, + setItem: (key, cache) => (storageMockCache[key] = cache), + }; + const cacheWrapper = new DefaultCacheWrapper(storageMock, loggerMock, null); wrappedFn = cacheWrapper.wrap(fn, { name: 'testCacheName' }); }); afterEach(() => { fn.reset(); - logger.error.reset(); - localStorage.clear(); + loggerMock.error.reset(); clock?.restore(); }); it('should throw error if uses without "name"', async () => { - const cacheWrapper = new CacheWrapper(localStorage, logger); - chai.expect(() => cacheWrapper.wrap(fn, { name: undefined })).to.throw( + const cacheWrapper = new DefaultCacheWrapper(storageMock, loggerMock, null); + chai.expect(() => cacheWrapper.wrap(fn, { name: undefined } as unknown as CacheParams)).to.throw( 'To wrap your function you should provide unique "name" to argument, to create hash-id of result of your function', ); }); @@ -97,7 +105,15 @@ describe('CacheWrapper', () => { }); it('should return the same value in case of concurrent invocation', async () => { - fn.withArgs().callsFake(() => new Promise((resolve) => setTimeout(() => resolve(data), 100))); + fn.withArgs().callsFake( + () => + new Promise((resolve) => + setTimeout(() => { + console.log('CALLED'); + return resolve(data); + }, 100), + ), + ); const [firstValue, secondValue, thirdValue] = await Promise.all([wrappedFn(), wrappedFn(), wrappedFn()]); @@ -106,13 +122,13 @@ describe('CacheWrapper', () => { }); it('should return a stale value while it requests new data in the background', async () => { - const cacheWrapper = new CacheWrapper(localStorage, logger, null, createHash); + const cacheWrapper = new DefaultCacheWrapper(storageMock, loggerMock, null, createHash); wrappedFn = cacheWrapper.wrap(fn, cacheParams); fn.withArgs(...fnArgs).returns(Promise.resolve(newData)); createHash .withArgs(cacheParams.name + cacheParams.cacheForSeconds + JSON.stringify(fnArgs)) .returns(cachedValueKey); - localStorage.setItem(cachedValueKey, JSON.stringify(prevCachedValue)); + storageMock.setItem(cachedValueKey, prevCachedValue); const firstValue = await wrappedFn(...fnArgs); await setImmediatePromise(); // Promise.race() in implemantion blocks cache update in same event loop tick so need to run additional tick in test @@ -156,14 +172,18 @@ describe('CacheWrapper', () => { const secondValue = await wrappedFn(); chai.expect(fn.calledTwice).to.be.true; chai.expect(secondValue).to.deep.equal({ cachedAt: 0, data }); - chai.expect(logger.error.called).to.be.false; + chai.expect(loggerMock.error.called).to.be.false; // timeout exceed await clock.tickAsync(cacheParams.cacheForSeconds * 1000); - chai.expect(logger.error.calledOnce).to.be.true; - chai.expect(logger.error.getCall(0).args[0].cause).to.be.instanceof(TimeoutError); - chai.expect(logger.error.getCall(0).args[0].message).to.eq('Error during cache update function execution'); - chai.expect(logger.error.getCall(0).args[0].cause.message).to.eq('Cache testCacheName update timeout 60s'); + chai.expect(loggerMock.error.calledOnce).to.be.true; + chai.expect((loggerMock.error.getCall(0).args[0] as ExtendedError).cause).to.be.instanceof(TimeoutError); + chai.expect((loggerMock.error.getCall(0).args[0] as ExtendedError).message).to.eq( + 'Error during cache update function execution', + ); + chai.expect((loggerMock.error.getCall(0).args[0] as ExtendedError).cause.message).to.eq( + 'Cache testCacheName update timeout 60s', + ); // update cache after error const thirdValue = await wrappedFn(); // this request will still return stale value since we do not wait for promises after 1st execution @@ -174,17 +194,17 @@ describe('CacheWrapper', () => { const fourthValue = await wrappedFn(); chai.expect(fn.calledThrice).to.be.true; chai.expect(fourthValue).to.deep.equal({ cachedAt: 7200, data: newData }); - chai.expect(logger.error.calledOnce).to.be.true; + chai.expect(loggerMock.error.calledOnce).to.be.true; }); describe('Multiple wrappers', () => { - let wrappedFirstFn; - let wrappedSecondFn; - let wrappedThirdFn; + let wrappedFirstFn: () => {}; + let wrappedSecondFn: () => {}; + let wrappedThirdFn: () => {}; describe('with same name and cacheForSeconds', () => { beforeEach(() => { - const cacheWrapper = new CacheWrapper(localStorage, logger); + const cacheWrapper = new DefaultCacheWrapper(storageMock, loggerMock, null); wrappedFirstFn = cacheWrapper.wrap(fn, { name: 'testCacheName' }); wrappedSecondFn = cacheWrapper.wrap(fn, { name: 'testCacheName' }); @@ -216,12 +236,12 @@ describe('CacheWrapper', () => { }); describe('with same name and cacheForSeconds', () => { - let fn; + let fn: sinon.SinonStub; beforeEach(() => { fn = sinon.stub(); - const cacheWrapper = new CacheWrapper(localStorage, logger); + const cacheWrapper = new DefaultCacheWrapper(storageMock, loggerMock, null); wrappedFirstFn = cacheWrapper.wrap(fn, { name: 'testCacheName', cacheForSeconds: 100 }); wrappedSecondFn = cacheWrapper.wrap(fn, { name: 'testCacheName', cacheForSeconds: 200 }); @@ -241,7 +261,7 @@ describe('CacheWrapper', () => { }); describe('Correct cache date', () => { - let clock; + let clock: sinon.SinonFakeTimers; beforeEach(() => (clock = sinon.useFakeTimers())); @@ -250,12 +270,13 @@ describe('CacheWrapper', () => { it('should set correct date now when saving new data to cache', async () => { const setItem = sinon.stub(); - const cacheWrapper = new CacheWrapper( + const cacheWrapper = new DefaultCacheWrapper( { setItem, getItem: () => null, }, - logger, + loggerMock, + null, ); wrappedFn = cacheWrapper.wrap( @@ -269,25 +290,19 @@ describe('CacheWrapper', () => { ); await wrappedFn(); - - chai.expect( - setItem.calledWith( - sinon.match.any, - JSON.stringify({ - data: 'data', - cachedAt: 50, - }), - ), - ).to.be.true; + sinon.assert.calledWith(setItem, sinon.match.any, { + data: 'data', + cachedAt: 50, + }); }); }); describe('when requesting new data fails in the foreground but cache has a stale value', () => { beforeEach(() => { - const cacheWrapper = new CacheWrapper(localStorage, logger, null, createHash); + const cacheWrapper = new DefaultCacheWrapper(storageMock, loggerMock, undefined, createHash); wrappedFn = cacheWrapper.wrap(fn, cacheParams); fn.withArgs(...fnArgs).returns(Promise.reject(fnError)); - localStorage.setItem(cachedValueKey, JSON.stringify(prevCachedValue)); + storageMock.setItem(cachedValueKey, prevCachedValue); createHash.withArgs(JSON.stringify(fnArgs)).returns(cachedValueKey); }); @@ -304,9 +319,11 @@ describe('CacheWrapper', () => { await wrappedFn(...fnArgs); await setImmediatePromise(); // Promise.race blocks promise resolving is same event loop tick, need to switch it manually - chai.expect(logger.error.calledOnce).to.be.true; - chai.expect(logger.error.getCall(0).args[0]).to.be.instanceof(Error); - chai.expect(logger.error.getCall(0).args[0].message).to.eq('Error during cache update function execution'); + chai.expect(loggerMock.error.calledOnce).to.be.true; + chai.expect(loggerMock.error.getCall(0).args[0]).to.be.instanceof(Error); + chai.expect((loggerMock.error.getCall(0).args[0] as ExtendedError).message).to.eq( + 'Error during cache update function execution', + ); chai.expect(fn.calledOnce).to.be.true; }); }); diff --git a/ilc/common/CacheWrapper.js b/ilc/common/DefaultCacheWrapper.ts similarity index 62% rename from ilc/common/CacheWrapper.js rename to ilc/common/DefaultCacheWrapper.ts index be0d2eaa9..791b6044f 100644 --- a/ilc/common/CacheWrapper.js +++ b/ilc/common/DefaultCacheWrapper.ts @@ -1,45 +1,58 @@ -const { withTimeout, extendError } = require('./utils'); +import { Logger } from 'ilc-plugins-sdk'; +import { CacheHashFn, CacheParams, CacheResult, CacheStorage, CacheWrapper } from './types/CacheWrapper'; +import { Context } from './types/Context'; +import { extendError, withTimeout } from './utils'; -const errors = {}; -errors.CacheWrapperError = extendError('CacheWrapperError', { defaultData: {} }); +const CacheWrapperError = extendError('CacheWrapperError', { defaultData: {} }); -class CacheWrapper { - #cacheRenewPromise = {}; +type CachePromises = { + [k: string]: Promise; +}; - constructor(localStorage, logger, context, createHash = hashFn) { - this.logger = logger; - this.context = context; - this.storage = localStorage; - this.createHash = createHash; - } +const defaultHashFn: CacheHashFn = (str) => { + for (var i = 0, h = 0xdeadbeef; i < str.length; i++) h = Math.imul(h ^ str.charCodeAt(i), 2654435761); + return ((h ^ (h >>> 16)) >>> 0).toString(); +}; + +export class DefaultCacheWrapper implements CacheWrapper { + private cacheRenewPromises: CachePromises = {}; + + constructor( + private readonly storage: CacheStorage, + private readonly logger: Logger, + private readonly context?: Context | null, + private readonly createHash: CacheHashFn = defaultHashFn, + ) {} - #getCache(hash) { - const cache = this.storage.getItem(hash); - return cache ? JSON.parse(cache) : null; + private getCache(hash: string): CacheResult | null { + return this.storage.getItem(hash); } - #setCache(hash, cache) { - this.storage.setItem(hash, JSON.stringify(cache)); + private setCache(hash: string, cache: CacheResult): void { + this.storage.setItem(hash, cache); } - #nowInSec() { + private nowInSec(): number { return Math.floor(Date.now() / 1000); } - #logMessage(str) { + private logMessage(str: string): string { return `[ILC Cache]: ${str}`; } - #getRequestId() { + private getRequestId(): string | undefined { if (this.context) { const contextStore = this.context.getStore(); - return contextStore?.get('reqId'); + return contextStore?.get('requestId'); } return undefined; } - wrap(fn, cacheParams) { + public wrap( + fn: (...args: U) => Promise, + cacheParams: CacheParams, + ): (...args: U) => Promise> { const { name: memoName, cacheForSeconds = 60 } = cacheParams; if (typeof memoName !== 'string' || memoName.length === 0) { @@ -49,17 +62,17 @@ class CacheWrapper { } return (...args) => { - const now = this.#nowInSec(); + const now = this.nowInSec(); const hash = this.createHash(`${memoName}${cacheForSeconds}${JSON.stringify(args)}`); const logInfo = { now, hash, memoName, - id: this.#getRequestId(), + id: this.getRequestId(), }; - const cache = this.#getCache(hash); + const cache = this.getCache(hash); const cacheIsActual = cache !== null && cache.cachedAt >= now - cacheForSeconds; // Return cache if actual @@ -69,13 +82,13 @@ class CacheWrapper { ...logInfo, cachedAt: cache.cachedAt, }, - this.#logMessage('Item read from cache. Cache is actual'), + this.logMessage('Item read from cache. Cache is actual'), ); return Promise.resolve(cache); } - const cacheRenewInProgress = this.#cacheRenewPromise[hash] !== undefined; + const cacheRenewInProgress = this.cacheRenewPromises[hash] !== undefined; if (cacheRenewInProgress) { // If cache is stale, but renew is in progress - return stale cache @@ -86,13 +99,13 @@ class CacheWrapper { ...logInfo, cachedAt: cache.cachedAt, }, - this.#logMessage('Item read from cache. Invalidation is in progress. Cache is stale'), + this.logMessage('Item read from cache. Invalidation is in progress. Cache is stale'), ); return Promise.resolve(cache); } - return this.#cacheRenewPromise[hash]; + return this.cacheRenewPromises[hash]; } this.logger.info( @@ -100,26 +113,26 @@ class CacheWrapper { ...logInfo, cachedAt: (cache || {}).cachedAt, }, - this.#logMessage('Invalidation started'), + this.logMessage('Invalidation started'), ); // Start cache renew - this.#cacheRenewPromise[hash] = withTimeout( + this.cacheRenewPromises[hash] = withTimeout( fn(...args), cacheForSeconds * 1000, `Cache ${memoName} update timeout ${cacheForSeconds}s`, ) .then((data) => { - const now = this.#nowInSec(); + const now = this.nowInSec(); const renewedCache = { data, cachedAt: now, }; - this.#setCache(hash, renewedCache); + this.setCache(hash, renewedCache); - delete this.#cacheRenewPromise[hash]; + delete this.cacheRenewPromises[hash]; this.logger.info( { @@ -127,20 +140,20 @@ class CacheWrapper { now, cachedAt: renewedCache.cachedAt, }, - this.#logMessage('Invalidation finished'), + this.logMessage('Invalidation finished'), ); return renewedCache; }) .catch((err) => { - delete this.#cacheRenewPromise[hash]; + delete this.cacheRenewPromises[hash]; if (cache === null) { this.logger.info( { ...logInfo, }, - this.#logMessage('Invalidation error'), + this.logMessage('Invalidation error'), ); // As someone is waiting for promise - just reject it @@ -148,7 +161,7 @@ class CacheWrapper { } else { // Here no one waiting for this promise anymore, thrown error would cause // unhandled promise rejection - const error = new errors.CacheWrapperError({ + const error = new CacheWrapperError({ cause: err, message: 'Error during cache update function execution', }); @@ -164,7 +177,7 @@ class CacheWrapper { ...logInfo, cachedAt: cache.cachedAt, }, - this.#logMessage( + this.logMessage( 'Item read from cache. Invalidation is in progress since current request. Cache is stale', ), ); @@ -176,17 +189,10 @@ class CacheWrapper { { ...logInfo, }, - this.#logMessage('Item read from API. Can not find anything in stale cache'), + this.logMessage('Item read from API. Can not find anything in stale cache'), ); - return this.#cacheRenewPromise[hash]; + return this.cacheRenewPromises[hash]; }; } } - -function hashFn(str) { - for (var i = 0, h = 0xdeadbeef; i < str.length; i++) h = Math.imul(h ^ str.charCodeAt(i), 2654435761); - return ((h ^ (h >>> 16)) >>> 0).toString(); -} - -module.exports = CacheWrapper; diff --git a/ilc/common/MemoryCacheStorage.ts b/ilc/common/MemoryCacheStorage.ts new file mode 100644 index 000000000..8ae41be1e --- /dev/null +++ b/ilc/common/MemoryCacheStorage.ts @@ -0,0 +1,12 @@ +import { CacheResult, CacheStorage } from './types/CacheWrapper'; + +export class MemoryCacheStorage implements CacheStorage { + private readonly cache: Record> = {}; + + getItem(key: string): CacheResult | null { + return this.cache[key] ?? null; + } + setItem(key: string, cache: CacheResult): void { + this.cache[key] = cache; + } +} diff --git a/ilc/common/localStorage.js b/ilc/common/localStorage.js deleted file mode 100644 index dc0f60c27..000000000 --- a/ilc/common/localStorage.js +++ /dev/null @@ -1,60 +0,0 @@ -// https://git.coolaj86.com/coolaj86/local-storage.js -// forked because "if(global.localStorage)" throw error if localStorage is disabled in browser - -// NOTE: -// this varies from actual localStorage in some subtle ways - -// also, there is no persistence -// TODO persist -(function () { - 'use strict'; - - var db; - - function LocalStorage() { - console.warn('Since localStorage is disabled or unsupported in the current browser we use polyfill'); - } - db = LocalStorage; - - db.prototype.getItem = function (key) { - if (this.hasOwnProperty(key)) { - return String(this[key]); - } - return null; - }; - - db.prototype.setItem = function (key, val) { - this[key] = String(val); - }; - - db.prototype.removeItem = function (key) { - delete this[key]; - }; - - db.prototype.clear = function () { - var self = this; - Object.keys(self).forEach(function (key) { - self[key] = undefined; - delete self[key]; - }); - }; - - db.prototype.key = function (i) { - i = i || 0; - return Object.keys(this)[i]; - }; - - db.prototype.__defineGetter__('length', function () { - return Object.keys(this).length; - }); - - //https://github.com/Modernizr/Modernizr/blob/master/feature-detects/storage/localstorage.js - try { - var mod = 'testLocalStorage'; - localStorage.setItem(mod, mod); - localStorage.removeItem(mod); - module.exports = localStorage; - } catch (e) { - module.exports = new LocalStorage(); - } -})(); diff --git a/ilc/common/localStorage.spec.js b/ilc/common/localStorage.spec.js deleted file mode 100644 index a2aee305a..000000000 --- a/ilc/common/localStorage.spec.js +++ /dev/null @@ -1,59 +0,0 @@ -import chai from 'chai'; -import localStorage from './localStorage'; - -describe('localStorage', () => { - afterEach(() => { - localStorage.clear(); - }); - - it('should return null if empty', async () => { - chai.expect(localStorage.getItem('key')).to.equal(null); - }); - - it('should return key name', async () => { - localStorage.setItem('a', 1); - - chai.expect(localStorage.key(0)).to.equal('a'); - }); - - it('should return item', async () => { - localStorage.setItem('b', '2'); - - chai.expect(localStorage.getItem('b')).to.equal('2'); - }); - - it('should return string if passed number', async () => { - localStorage.setItem('a', 1); - - chai.expect(localStorage.getItem('a')).to.equal('1'); - }); - - it('should return items length', async () => { - localStorage.setItem('a', 1); - localStorage.setItem('b', '2'); - - chai.expect(localStorage.length).to.equal(2); - }); - - it('should remove item', async () => { - localStorage.setItem('b', '2'); - chai.expect(localStorage.getItem('b')).to.equal('2'); - - localStorage.removeItem('b'); - chai.expect(localStorage.getItem('b')).to.equal(null); - chai.expect(localStorage.length).to.equal(0); - }); - - it('should clear storage', async () => { - localStorage.setItem('a', 1); - localStorage.setItem('b', '2'); - chai.expect(localStorage.getItem('a')).to.equal('1'); - chai.expect(localStorage.getItem('b')).to.equal('2'); - - localStorage.clear(); - - chai.expect(localStorage.getItem('a')).to.equal(null); - chai.expect(localStorage.getItem('b')).to.equal(null); - chai.expect(localStorage.length).to.equal(0); - }); -}); diff --git a/ilc/server/types/CacheWrapper.ts b/ilc/common/types/CacheWrapper.ts similarity index 57% rename from ilc/server/types/CacheWrapper.ts rename to ilc/common/types/CacheWrapper.ts index c823b5a52..7e3f7ca08 100644 --- a/ilc/server/types/CacheWrapper.ts +++ b/ilc/common/types/CacheWrapper.ts @@ -3,10 +3,16 @@ export interface CacheResult { cachedAt: number; } -interface CacheParams { +export interface CacheParams { name: string; cacheForSeconds?: number; } +export type CacheHashFn = (value: string) => string; + +export interface CacheStorage { + getItem(key: string): CacheResult | null; + setItem(key: string, cache: CacheResult): void; +} export interface CacheWrapper { wrap( diff --git a/ilc/common/types/Context.ts b/ilc/common/types/Context.ts new file mode 100644 index 000000000..39ce793a3 --- /dev/null +++ b/ilc/common/types/Context.ts @@ -0,0 +1,5 @@ +import type { AsyncLocalStorage } from 'node:async_hooks'; + +export type ContextKey = 'requestId' | 'url' | 'domain' | 'path' | 'protocol'; +export type Store = Map; +export type Context = AsyncLocalStorage; diff --git a/ilc/server/app.js b/ilc/server/app.js index b225f3473..7ef6cc0fd 100644 --- a/ilc/server/app.js +++ b/ilc/server/app.js @@ -1,11 +1,12 @@ +import config from 'config'; +import fastify from 'fastify'; import { AsyncResource } from 'node:async_hooks'; +import { pingPluginFactroy } from './routes/pingPluginFactory'; import { renderTemplateHandlerFactory } from './routes/renderTemplateHandlerFactory'; import { wildcardRequestHandlerFactory } from './routes/wildcardRequestHandlerFactory'; +import { errorHandlerFactory } from './errorHandler/factory'; -const config = require('config'); -const fastify = require('fastify'); const serveStatic = require('./serveStatic'); -const errorHandlingService = require('./errorHandler/factory'); const i18n = require('./i18n'); const Application = require('./application/application'); const reportingPluginManager = require('./plugins/reportingPlugin'); @@ -17,7 +18,7 @@ const { isStaticFile, isHealthCheck } = require('./utils/utils'); */ module.exports = (registryService, pluginManager, context) => { const reportingPlugin = reportingPluginManager.getInstance(); - + const errorHandler = errorHandlerFactory(); const appConfig = Application.getConfig(reportingPlugin); const logger = reportingPluginManager.getLogger(); const accessLogger = new AccessLogger(config, logger); @@ -95,18 +96,22 @@ module.exports = (registryService, pluginManager, context) => { app.use(config.get('static.internalUrl'), serveStatic(config.get('productionMode'))); } - app.register(require('./ping')); + const pingPlugin = pingPluginFactroy(registryService); + app.register(pingPlugin); - app.get('/_ilc/api/v1/registry/template/:templateName', renderTemplateHandlerFactory(registryService)); + app.get( + '/_ilc/api/v1/registry/template/:templateName', + renderTemplateHandlerFactory(registryService, errorHandler), + ); // Route to test 500 page appearance app.get('/_ilc/500', async () => { throw new Error('500 page test error'); }); - app.all('*', wildcardRequestHandlerFactory(logger, registryService, pluginManager)); + app.all('*', wildcardRequestHandlerFactory(logger, registryService, errorHandler, pluginManager)); - app.setErrorHandler(errorHandlingService.handleError); + app.setErrorHandler(errorHandler.handleError); return app; }; diff --git a/ilc/server/app.spec.js b/ilc/server/app.spec.js index fc591141d..1521bd4f4 100644 --- a/ilc/server/app.spec.js +++ b/ilc/server/app.spec.js @@ -1,11 +1,8 @@ const chai = require('chai'); const supertest = require('supertest'); -const nock = require('nock'); -const config = require('config'); const helpers = require('../tests/helpers'); const { context } = require('./context/context'); const createApp = require('./app'); -const sinon = require('sinon'); async function createTestServer(mockRegistryOptions = {}, mockPluginOptions = {}) { const app = createApp( diff --git a/ilc/server/errorHandler/ErrorHandler.spec.js b/ilc/server/errorHandler/ErrorHandler.spec.js index 5c252ae6b..98b33ca4d 100644 --- a/ilc/server/errorHandler/ErrorHandler.spec.js +++ b/ilc/server/errorHandler/ErrorHandler.spec.js @@ -6,7 +6,6 @@ const config = require('config'); const fs = require('fs'); const path = require('path'); const { StatusCodes, ReasonPhrases } = require('http-status-codes'); -const localStorage = require('../../common/localStorage'); const helpers = require('../../tests/helpers'); const { context } = require('../context/context'); const ErrorHandler = require('./ErrorHandler'); @@ -23,7 +22,7 @@ describe('ErrorHandler', () => { let server; let address; - before(async () => { + beforeEach(async () => { app = createApp(helpers.getRegistryMock(), helpers.getPluginManagerMock(), context); await app.ready(); app.server.listen(0); @@ -35,10 +34,6 @@ describe('ErrorHandler', () => { }); afterEach(() => { - localStorage.clear(); - }); - - after(() => { app.server.close(); }); diff --git a/ilc/server/errorHandler/factory.js b/ilc/server/errorHandler/factory.js index d35c77f14..684f4dca0 100644 --- a/ilc/server/errorHandler/factory.js +++ b/ilc/server/errorHandler/factory.js @@ -1,9 +1,10 @@ -const newrelic = require('newrelic'); +import newrelic from 'newrelic'; +import reportPlugin from '../plugins/reportingPlugin'; +import { registryFactory } from '../registry/factory'; +import ErrorHandler from './ErrorHandler'; -const registryService = require('../registry/factory'); -const ErrorHandler = require('./ErrorHandler'); - -const reportingPluginManager = require('../plugins/reportingPlugin'); -const logger = reportingPluginManager.getLogger(); - -module.exports = new ErrorHandler(registryService, newrelic, logger); +export const errorHandlerFactory = () => { + const logger = reportPlugin.getLogger(); + const registryService = registryFactory(); + return new ErrorHandler(registryService, newrelic, logger); +}; diff --git a/ilc/server/index.js b/ilc/server/index.js index 78266c382..3e0a08cd9 100644 --- a/ilc/server/index.js +++ b/ilc/server/index.js @@ -1,4 +1,5 @@ import 'source-map-support/register'; +import { registryFactory } from './registry/factory'; const path = require('path'); const { context } = require('./context/context'); @@ -6,10 +7,9 @@ const { context } = require('./context/context'); process.env.NODE_CONFIG_DIR = path.resolve(__dirname, '../config'); require('newrelic'); -const registryService = require('./registry/factory'); const pluginManager = require('./plugins/pluginManager'); const runServer = require('./server'); const createApp = require('./app'); -runServer(createApp(registryService, pluginManager, context)); +runServer(createApp(registryFactory(), pluginManager, context)); diff --git a/ilc/server/ping.js b/ilc/server/ping.js deleted file mode 100644 index 9e938d943..000000000 --- a/ilc/server/ping.js +++ /dev/null @@ -1,12 +0,0 @@ -const config = require('config'); -const registryService = require('./registry/factory'); - -module.exports = function (fastify, opts, done) { - const healthCheckUrl = config.get('healthCheck.url'); - fastify.get(healthCheckUrl, async (req, res) => { - await registryService.preheat(); - res.status(200).send('pong'); - }); - - done(); -}; diff --git a/ilc/server/registry/Registry.js b/ilc/server/registry/Registry.js index 2882a48e1..1123affcd 100644 --- a/ilc/server/registry/Registry.js +++ b/ilc/server/registry/Registry.js @@ -17,7 +17,7 @@ module.exports = class Registry { /** * @param {string} address - registry address. Ex: http://registry:8080/ - * @param {Function} cacheWrapper - cache provider + * @param {Class} cacheWrapper - cache provider * @param {Object} logger - log provider that implements "console" interface */ constructor(address, cacheWrapper, logger) { diff --git a/ilc/server/registry/factory.js b/ilc/server/registry/factory.js deleted file mode 100644 index 80e7225cb..000000000 --- a/ilc/server/registry/factory.js +++ /dev/null @@ -1,9 +0,0 @@ -const config = require('config'); -const localStorage = require('../../common/localStorage'); -const Registry = require('./Registry'); -const CacheWrapper = require('../../common/CacheWrapper'); -const reportingPluginManager = require('../plugins/reportingPlugin'); -const { context } = require('../context/context'); -const logger = reportingPluginManager.getLogger(); - -module.exports = new Registry(config.get('registry.address'), new CacheWrapper(localStorage, logger, context), logger); diff --git a/ilc/server/registry/factory.ts b/ilc/server/registry/factory.ts new file mode 100644 index 000000000..b89e6bdc9 --- /dev/null +++ b/ilc/server/registry/factory.ts @@ -0,0 +1,15 @@ +import config from 'config'; +import { DefaultCacheWrapper } from '../../common/DefaultCacheWrapper'; +import { MemoryCacheStorage } from '../../common/MemoryCacheStorage'; +import { context } from '../context/context'; +import reportPlugin from '../plugins/reportingPlugin'; +import Registry from './Registry'; + +export const registryFactory = () => { + const logger = reportPlugin.getLogger(); + return new Registry( + config.get('registry.address'), + new DefaultCacheWrapper(new MemoryCacheStorage(), logger, context), + logger, + ); +}; diff --git a/ilc/server/routes/pingPluginFactory.ts b/ilc/server/routes/pingPluginFactory.ts new file mode 100644 index 000000000..baf341ffb --- /dev/null +++ b/ilc/server/routes/pingPluginFactory.ts @@ -0,0 +1,23 @@ +import config from 'config'; +import type { FastifyReply, FastifyRequest, Plugin, RegisterOptions } from 'fastify'; +import type http from 'http'; +import { Registry } from '../types/Registry'; + +export function pingPluginFactroy( + registry: Registry, +): Plugin< + http.Server, + http.IncomingMessage, + http.ServerResponse, + RegisterOptions +> { + return (fastify, opts, done) => { + const healthCheckUrl = config.get('healthCheck.url'); + fastify.get(healthCheckUrl, async (req: FastifyRequest, res: FastifyReply) => { + await registry.preheat(); + res.status(200).send('pong'); + }); + + done(); + }; +} diff --git a/ilc/server/routes/renderTemplateHandlerFactory.ts b/ilc/server/routes/renderTemplateHandlerFactory.ts index e1a4b7e21..2f92a9d38 100644 --- a/ilc/server/routes/renderTemplateHandlerFactory.ts +++ b/ilc/server/routes/renderTemplateHandlerFactory.ts @@ -1,12 +1,14 @@ import type { RequestHandler } from 'fastify'; import { StatusCodes } from 'http-status-codes'; - -import errorHandlingService from '../errorHandler/factory'; import { NotFoundRegistryError, ValidationRegistryError } from '../registry/errors'; -import Registry from '../registry/Registry'; +import { ErrorHandler } from '../types/ErrorHandler'; import { PatchedHttpRequest } from '../types/PatchedHttpRequest'; +import { Registry } from '../types/Registry'; -export function renderTemplateHandlerFactory(registryService: Registry): RequestHandler { +export function renderTemplateHandlerFactory( + registryService: Registry, + errorHandlingService: ErrorHandler, +): RequestHandler { return async (req, reply) => { const currentDomain = req.hostname; const locale = req.raw.ilcState?.locale; diff --git a/ilc/server/routes/wildcardRequestHandlerFactory.ts b/ilc/server/routes/wildcardRequestHandlerFactory.ts index 8870c515c..e47c1e5a4 100644 --- a/ilc/server/routes/wildcardRequestHandlerFactory.ts +++ b/ilc/server/routes/wildcardRequestHandlerFactory.ts @@ -1,9 +1,10 @@ import config from 'config'; import newrelic from 'newrelic'; +import type { RequestHandler } from 'fastify'; +import type { Logger, PluginManager } from 'ilc-plugins-sdk'; import { SlotCollection } from '../../common/Slot/SlotCollection'; import UrlProcessor from '../../common/UrlProcessor'; -import errorHandlingService from '../errorHandler/factory'; import GuardManager from '../GuardManager'; import i18n from '../i18n'; import CspBuilderService from '../services/CspBuilderService'; @@ -11,15 +12,14 @@ import tailorFactory from '../tailor/factory'; import mergeConfigs from '../tailor/merge-configs'; import parseOverrideConfig from '../tailor/parse-override-config'; import ServerRouter from '../tailor/server-router'; +import { ErrorHandler } from '../types/ErrorHandler'; import { PatchedHttpRequest } from '../types/PatchedHttpRequest'; import { Registry, TransformedRegistryConfig } from '../types/Registry'; -import type { RequestHandler } from 'fastify'; -import type { Logger, PluginManager } from 'ilc-plugins-sdk'; - export function wildcardRequestHandlerFactory( logger: Logger, registryService: Registry, + errorHandlingService: ErrorHandler, pluginManager: PluginManager, ): RequestHandler { const guardManager = new GuardManager(pluginManager); @@ -31,6 +31,7 @@ export function wildcardRequestHandlerFactory( const tailor = tailorFactory( registryService, + errorHandlingService, config.get('cdnUrl'), config.get('newrelic.customClientJsWrapper'), autoInjectNrMonitoring, diff --git a/ilc/server/tailor/factory.js b/ilc/server/tailor/factory.js index 3485faf85..1962e719e 100644 --- a/ilc/server/tailor/factory.js +++ b/ilc/server/tailor/factory.js @@ -6,7 +6,6 @@ const newrelic = require('newrelic'); const Tailor = require('@namecheap/tailorx'); const fetchTemplate = require('./fetch-template'); const filterHeaders = require('./filter-headers'); -const errorHandlingService = require('../errorHandler/factory'); const errorHandlerSetup = require('./error-handler'); const fragmentHooks = require('./fragment-hooks'); const ConfigsInjector = require('./configs-injector'); @@ -15,6 +14,7 @@ const requestFragment = require('./request-fragment'); module.exports = function ( registryService, + errorHandlingService, cdnUrl, nrCustomClientJsWrapper = null, nrAutomaticallyInjectClientScript = true, diff --git a/ilc/server/types/ErrorHandler.ts b/ilc/server/types/ErrorHandler.ts new file mode 100644 index 000000000..c34aa77cf --- /dev/null +++ b/ilc/server/types/ErrorHandler.ts @@ -0,0 +1,7 @@ +import { FastifyReply } from 'fastify'; +import http from 'http'; + +export interface ErrorHandler { + noticeError(error: unknown, attributes: Record): void; + handleClientError(reply: FastifyReply, error: unknown, code: number): void; +} diff --git a/ilc/server/types/Registry.ts b/ilc/server/types/Registry.ts index e343a9470..4124e1d38 100644 --- a/ilc/server/types/Registry.ts +++ b/ilc/server/types/Registry.ts @@ -1,6 +1,6 @@ import type { Logger } from 'ilc-plugins-sdk'; import type { RegistryConfig } from './RegistryConfig'; -import { CacheResult } from './CacheWrapper'; +import { CacheResult } from '../../common/types/CacheWrapper'; export interface TransformedRegistryConfig extends RegistryConfig { // TODO