Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent chrome storage resolving before entries are set #126

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions src/cache/chrome-storage-async-map.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/// <reference types="chrome"/>

import { applicationLogger } from '@eppo/js-client-sdk-common';

import ChromeStorageAsyncMap from './chrome-storage-async-map';

describe('ChromeStorageAsyncMap', () => {
const mockStorage = {
set: jest.fn(),
} as unknown as chrome.storage.StorageArea;

let storageMap: ChromeStorageAsyncMap<string>;

beforeEach(() => {
jest.clearAllMocks();
jest.spyOn(applicationLogger, 'warn').mockImplementation();
(mockStorage.set as jest.Mock).mockImplementation(() => Promise.resolve());
storageMap = new ChromeStorageAsyncMap(mockStorage);
});

describe('set', () => {
it('should store and retrieve values correctly', async () => {
const key = 'testKey';
const value = 'testValue';

await storageMap.set(key, value);
expect(mockStorage.set).toHaveBeenCalledWith({ [key]: value });
});

it('should propagate storage errors', async () => {
const key = 'testKey';
const value = 'testValue';
const error = new Error('Storage error');

(mockStorage.set as jest.Mock).mockRejectedValue(error);

await expect(storageMap.set(key, value)).rejects.toThrow(error);
expect(applicationLogger.warn).toHaveBeenCalledWith(
'Chrome storage write failed for key:',
key,
error,
);
});

it('should handle multiple keys independently', async () => {
const key1 = 'testKey1';
const value1 = 'testValue1';
const key2 = 'testKey2';
const value2 = 'testValue2';

await storageMap.set(key1, value1);
await storageMap.set(key2, value2);

expect(mockStorage.set).toHaveBeenCalledWith({ [key1]: value1 });
expect(mockStorage.set).toHaveBeenCalledWith({ [key2]: value2 });
});
});
});
11 changes: 8 additions & 3 deletions src/cache/chrome-storage-async-map.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { AsyncMap } from '@eppo/js-client-sdk-common';
import { applicationLogger, AsyncMap } from '@eppo/js-client-sdk-common';

/** Chrome storage-backed {@link AsyncMap}. */
export default class ChromeStorageAsyncMap<T> implements AsyncMap<string, T> {
Expand All @@ -18,7 +18,12 @@ export default class ChromeStorageAsyncMap<T> implements AsyncMap<string, T> {
return await this.storage.get(null);
}

async set(key: string, value: T) {
await this.storage.set({ [key]: value });
async set(key: string, value: T): Promise<void> {
try {
await this.storage.set({ [key]: value });
} catch (error) {
applicationLogger.warn('Chrome storage write failed for key:', key, error);
throw error;
}
}
}
22 changes: 16 additions & 6 deletions src/string-valued.store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,18 @@ export interface IStringStorageEngine {
*/
export class StringValuedAsyncStore<T> implements IAsyncStore<T> {
private initialized = false;
private isInitializing = false;

public constructor(private storageEngine: IStringStorageEngine, private cooldownSeconds = 0) {}

public isInitialized(): boolean {
return this.initialized;
}

public isInProcessOfInitializing(): boolean {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO add isInProcessOfInitializing to the IAsyncStore interface

return this.isInitializing;
}

public async isExpired(): Promise<boolean> {
if (!this.cooldownSeconds) {
return true;
Expand All @@ -47,11 +52,16 @@ export class StringValuedAsyncStore<T> implements IAsyncStore<T> {
}

public async setEntries(entries: Record<string, T>): Promise<void> {
// String-based storage takes a dictionary of key-value string pairs,
// so we write the entire configuration and meta to a single location for each.
await this.storageEngine.setContentsJsonString(JSON.stringify(entries));
const updatedMeta = { lastUpdatedAtMs: new Date().getTime() };
await this.storageEngine.setMetaJsonString(JSON.stringify(updatedMeta));
this.initialized = true;
try {
this.isInitializing = true;
// String-based storage takes a dictionary of key-value string pairs,
// so we write the entire configuration and meta to a single location for each.
await this.storageEngine.setContentsJsonString(JSON.stringify(entries));
const updatedMeta = { lastUpdatedAtMs: new Date().getTime() };
await this.storageEngine.setMetaJsonString(JSON.stringify(updatedMeta));
this.initialized = true;
} finally {
this.isInitializing = false;
}
}
}
Loading