-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: eliminate code duplication across storage adapters, persistence strategies, and React provider #104
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,13 +53,108 @@ test('BrowserStorageAdapter gracefully degrades when window/localStorage are una | |
| const adapter = new BrowserStorageAdapter(); | ||
| assert.equal(adapter.getItem('missing'), null); | ||
| assert.doesNotThrow(() => adapter.setItem('k', 'v')); | ||
| assert.doesNotThrow(() => adapter.removeItem('k')); | ||
| } finally { | ||
| if (originalWindow !== undefined) { | ||
| globalThis.window = originalWindow; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| test('BrowserStorageAdapter: removeItem() removes a namespaced key from localStorage', () => { | ||
| const store = new Map(); | ||
| globalThis.window = { | ||
| localStorage: { | ||
| getItem: (k) => store.get(k) ?? null, | ||
|
Comment on lines
+64
to
+68
|
||
| setItem: (k, v) => store.set(k, v), | ||
| removeItem: (k) => store.delete(k), | ||
| }, | ||
| }; | ||
| try { | ||
| const adapter = new BrowserStorageAdapter(); | ||
| adapter.setItem('my-key', 'my-value'); | ||
| assert.equal(adapter.getItem('my-key'), 'my-value'); | ||
| adapter.removeItem('my-key'); | ||
| assert.equal(adapter.getItem('my-key'), null, 'value must be absent after removeItem'); | ||
| // Confirm the namespaced key (not the bare key) was removed. | ||
| assert.equal(store.has('passiveintent:my-key'), false); | ||
| assert.equal(store.has('my-key'), false); | ||
| } finally { | ||
| delete globalThis.window; | ||
| } | ||
| }); | ||
|
|
||
| test('BrowserStorageAdapter: getItem() migrates legacy unprefixed key to namespaced key on first read', () => { | ||
| const store = new Map(); | ||
| // Simulate an old SDK installation that wrote the key without a namespace prefix. | ||
| store.set('intent-key', 'legacy-value'); | ||
| globalThis.window = { | ||
| localStorage: { | ||
| getItem: (k) => store.get(k) ?? null, | ||
|
Comment on lines
+87
to
+93
|
||
| setItem: (k, v) => store.set(k, v), | ||
| removeItem: (k) => store.delete(k), | ||
| }, | ||
| }; | ||
| try { | ||
| const adapter = new BrowserStorageAdapter(); | ||
| // First read must fall back to the unprefixed key and return the value. | ||
| assert.equal(adapter.getItem('intent-key'), 'legacy-value'); | ||
| // The value must now be stored under the namespaced key. | ||
| assert.equal(store.get('passiveintent:intent-key'), 'legacy-value', 'value must be migrated to namespaced key'); | ||
| // The legacy unprefixed key must be removed. | ||
| assert.equal(store.has('intent-key'), false, 'legacy key must be deleted after migration'); | ||
| // Subsequent reads must hit the namespaced key directly. | ||
| assert.equal(adapter.getItem('intent-key'), 'legacy-value'); | ||
| } finally { | ||
| delete globalThis.window; | ||
| } | ||
| }); | ||
|
|
||
| test('BrowserStorageAdapter: getItem() does NOT migrate legacy key for custom namespaces', () => { | ||
| const store = new Map(); | ||
| store.set('intent-key', 'legacy-value'); | ||
| globalThis.window = { | ||
| localStorage: { | ||
| getItem: (k) => store.get(k) ?? null, | ||
|
Comment on lines
+113
to
+118
|
||
| setItem: (k, v) => store.set(k, v), | ||
| removeItem: (k) => store.delete(k), | ||
| }, | ||
| }; | ||
| try { | ||
| const adapter = new BrowserStorageAdapter('my-mfe:'); | ||
| // Custom-namespace adapter must not touch the unprefixed key. | ||
| assert.equal(adapter.getItem('intent-key'), null); | ||
| assert.equal(store.has('intent-key'), true, 'legacy key must remain untouched'); | ||
| } finally { | ||
| delete globalThis.window; | ||
| } | ||
| }); | ||
|
|
||
| test('BrowserStorageAdapter: custom namespace prefixes keys independently from the default namespace', () => { | ||
| const store = new Map(); | ||
| globalThis.window = { | ||
| localStorage: { | ||
| getItem: (k) => store.get(k) ?? null, | ||
|
Comment on lines
+133
to
+137
|
||
| setItem: (k, v) => store.set(k, v), | ||
| removeItem: (k) => store.delete(k), | ||
| }, | ||
| }; | ||
| try { | ||
| const def = new BrowserStorageAdapter(); | ||
| const mfe = new BrowserStorageAdapter('checkout:'); | ||
| def.setItem('state', 'global'); | ||
| mfe.setItem('state', 'checkout'); | ||
| // Each adapter must only see its own namespaced value. | ||
| assert.equal(def.getItem('state'), 'global'); | ||
| assert.equal(mfe.getItem('state'), 'checkout'); | ||
| // Underlying keys must be stored with the correct prefixes. | ||
| assert.equal(store.get('passiveintent:state'), 'global'); | ||
| assert.equal(store.get('checkout:state'), 'checkout'); | ||
| } finally { | ||
| delete globalThis.window; | ||
| } | ||
| }); | ||
|
|
||
| test('BrowserTimerAdapter works with platform timers and monotonic fallback', () => { | ||
| const timer = new BrowserTimerAdapter(); | ||
| let fired = false; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated JSDoc for
save()says higher-layer error handling viaIntentEngine._persist()/onErroris the right place to observe quota/security failures, but this method intentionally swallows those errors in its own try/catch. As written, callers generally won’t observe these failures viaonErrorbecause nothing is thrown. Please adjust the comment to reflect the actual behavior/contract (swallowed errors are not observable upstream).