fix(electron-sdk): localstorage indexing#1262
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/browser size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
joker23
force-pushed
the
skz/sdk-2156/electron-localstorage
branch
2 times, most recently
from
1ba2c69 to
b327868
Compare
|
@cursor review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f084d37. Configure here.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| // Storage should be a singleton to support multiple instances of the SDK. This should have | ||
| // the same limitations as the browser sdk using the shared localStorage cache. | ||
| let instance: ElectronStorage | undefined; |
There was a problem hiding this comment.
To match JS we would likely need two layers here. This singleton holds a logger which means it will use the first logger encountered.
Where the JS storage is like an inner layer, and the outer layer isn't sharing. This implementation use a file, which is the equivalent of the singleton layer. Not to say we shouldn't use a singleton here, because the platform constraints are a little different, and this handles the cache consistency for us.
But we will likely want a nested approach.
There was a problem hiding this comment.
okay I think this is addressed with the latest changes
joker23
force-pushed
the
skz/sdk-2156/electron-localstorage
branch
from
38dc315 to
b342d55
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
joker23
force-pushed
the
skz/sdk-2156/electron-localstorage
branch
from
b342d55 to
81826d7
Compare
2 participants
This PR will fix the localstorage indexing by removing the previous nested structure:
To:
The new structure is more consistent with other SDKs by calculating the storage index purely out of the context canonical keys.
This PR will also serialize the write to this cache so that multiple instances of this SDK could exist.
Note
Medium Risk
Changes how all Electron SDK instances persist flag data by moving to a shared
ldcachefile and introducing singleton/queued flush behavior; this can affect cache isolation and persistence correctness across clients. Risk is mitigated by added tests, but storage/indexing regressions would impact offline/bootstrapped evaluations.Overview
Switches Electron SDK persistent flag/context caching from per-credential files (e.g.
ldcache-{credentialHash}) to a single sharedldcachefile via a singletonElectronStorage, aligning cache indexing purely with context-derived keys.ElectronStoragenow updates its in-memory cache first and serializes disk flushes through a queued atomic-write mechanism; initialization failures now surface as thrown errors (with cause) to the platform wrapper, which logs and swallows storage failures.Updates contract tests to reset the storage singleton between clients and removes the
singletoncapability flag, and adds coverage formaxCachedContextseviction/disabled caching plus newElectronPlatformerror-logging tests.Reviewed by Cursor Bugbot for commit 81826d7. Bugbot is set up for automated code reviews on this repo. Configure here.