perf: stop using localstorage polyfill on server side as it's contain…

…s serialization/deserialization on each call to cache

+ts refactoring

.gitignore

@@ -9,3 +9,4 @@ node_modules
 registry/lde/dbfiles
 .yalc
 yalc.lock
+.clinic

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,

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')),

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;

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<typeof value>(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));
+        });
+    });
+});

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<T>(key: string): CacheResult<T> {
+        const cache = this.storage.getItem(key);
+        return cache ? JSON.parse(cache) : null;
+    }
+
+    setItem(key: string, cache: CacheResult<unknown>): void {
+        this.storage.setItem(key, JSON.stringify(cache));
+    }
+}

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));

ilc/common/CacheWrapper.spec.js → 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<CacheResult<unknown>>;
+    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<Logger>;
 
     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<string, CacheResult<any>> = {};
+        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,22 +105,32 @@ 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()]);
+        console.log('🚀 ~ it ~ firstValue:', firstValue);
 
         chai.expect(firstValue).to.deep.equal(secondValue).and.to.deep.equal(thirdValue);
+        console.log('DSFDSFDSFFSDFS', fn.getCalls());
         chai.expect(fn.calledOnce).to.be.true;
     });
 
     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 +174,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 +196,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 +238,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 +263,7 @@ describe('CacheWrapper', () => {
     });
 
     describe('Correct cache date', () => {
-        let clock;
+        let clock: sinon.SinonFakeTimers;
 
         beforeEach(() => (clock = sinon.useFakeTimers()));
 
@@ -250,12 +272,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 +292,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 +321,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;
         });
     });