[PR #4799] [MERGED] fix(common): `PersistenceService` call site compat #4943

New issue

Closed

opened 2026-03-17 02:26:02 +03:00 by kerem · 0 comments

kerem commented

2026-03-17 02:26:02 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/hoppscotch/hoppscotch/pull/4799
Author: @CuriousCorrelation
Created: 3/2/2025
Status: ✅ Merged
Merged: 3/3/2025
Merged by: @jamesgeorge007

Base: next ← Head: fix/persistence-compat

📝 Commits (4)

b8daf69 fix(common): PersistenceService call site compat
96f9967 fix: lint
f75be09 fix(common): docs and deprecation warning for legacy
8572ed2 fix(common): default conf store-like to localStorage

📊 Changes

4 files changed (+137 additions, -27 deletions)

View changed files

📝 packages/hoppscotch-common/src/services/persistence/__tests__/index.spec.ts (+56 -0)
📝 packages/hoppscotch-common/src/services/persistence/index.ts (+71 -5)
📝 packages/hoppscotch-common/src/services/tab/graphql.ts (+5 -11)
📝 packages/hoppscotch-common/src/services/tab/rest.ts (+5 -11)

📄 Description

This PR fixes some of the inconsistencies in calls to PersistenceService around double serialization/deserialization.

Legacy PersistenceService calls are serialized at the call site, for example:

persistenceService.setLocalConfig("login_state", JSON.stringify(user))

Although kernel's Store.set automatically serializes value for storage (and Store.get deserializes automatically as well):

async set(namespace: string, key: string, value: StoredData): Promise<void> {
    const validated = StoredDataSchema.parse(value);
    // <platform specific serialization>
    this.notifyListeners(namespace, key, validated.data);
}

The legacy approach still works due to how platform-dependent serialization/deserialization handles string values specifically. When we call JSON.stringify(data) in legacy modules, we're converting an object to a JSON string.

When this string is passed to persistenceService.setLocalConfig(), it goes through the store implementation where <platform>.<stringify>() is used. When <platform>.<stringify>() receives a primitive string value (which we've already stringified), it simply treats it as a regular string value and serializes it accordingly.

During retrieval, <platform>.<parse>() deserializes this back into a string.

Basically, this works due to a "wrapping" pattern that the data goes through:

Object → JSON.stringify → string → <platform>.<stringify>() → stored string → <platform>.<parse>() → string → JSON.parse → Object

This is definitely not ideal:

It's inefficient
We lose type information
There's a chance of forgetting JSON.stringify (for legacy method calls)
Creates unnecessary storage bloat

This PR introduces new methods set, get and remove in order to incrementally refactor existing code to the correct usage while keeping the codebase stable for backwards compatibility.

For future changes, it's important to note that we have migrations in place for certain keys in PersistenceService. When possible, add variables to the migration mechanism to make sure data isn't lost midway. Not all changes necessitate migrations - for example, login_state can be replaced without a migration as it will simply prompt users to log back in.

However, some data is synced with the backend (either selfhosted or cloud):

platform.sync.settings.initSettingsSync(),
platform.sync.collections.initCollectionsSync(),
platform.sync.history.initHistorySync(),
platform.sync.environments.initEnvironmentsSync(),

These synchronized items definitely need migrations to ensure users don't lose their data, and we've provided those migrations in the service.

Note to Reviewers

There still exist sync calls to PersistenceService methods, especially in std/interceptors, sh-desktop/platform and similar legacy modules. Although these modules are not directly in use anymore, there may exist some static function calls that may be used by live modules indirectly, even though they don't depend on the kernel.

These modules are up for removal upon ongoing evaluation, so any PersistenceService in these modules can be safely ignored.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/hoppscotch/hoppscotch/pull/4799 **Author:** [@CuriousCorrelation](https://github.com/CuriousCorrelation) **Created:** 3/2/2025 **Status:** ✅ Merged **Merged:** 3/3/2025 **Merged by:** [@jamesgeorge007](https://github.com/jamesgeorge007) **Base:** `next` ← **Head:** `fix/persistence-compat` --- ### 📝 Commits (4) - [`b8daf69`](https://github.com/hoppscotch/hoppscotch/commit/b8daf699763f4a7057715430f3e7a32902c58a08) fix(common): `PersistenceService` call site compat - [`96f9967`](https://github.com/hoppscotch/hoppscotch/commit/96f996765dc32f5109c4538e44be7613a0cdd093) fix: lint - [`f75be09`](https://github.com/hoppscotch/hoppscotch/commit/f75be09ef3f2f392078556b60f3e24d1ef738c26) fix(common): docs and deprecation warning for legacy - [`8572ed2`](https://github.com/hoppscotch/hoppscotch/commit/8572ed238607d62eff5713fb305ef2f6736cd198) fix(common): default conf store-like to `localStorage` ### 📊 Changes **4 files changed** (+137 additions, -27 deletions) <details> <summary>View changed files</summary> 📝 `packages/hoppscotch-common/src/services/persistence/__tests__/index.spec.ts` (+56 -0) 📝 `packages/hoppscotch-common/src/services/persistence/index.ts` (+71 -5) 📝 `packages/hoppscotch-common/src/services/tab/graphql.ts` (+5 -11) 📝 `packages/hoppscotch-common/src/services/tab/rest.ts` (+5 -11) </details> ### 📄 Description This PR fixes some of the inconsistencies in calls to `PersistenceService` around double serialization/deserialization. Legacy `PersistenceService` calls are serialized at the call site, for example: ```typescript persistenceService.setLocalConfig("login_state", JSON.stringify(user)) ``` Although `kernel`'s `Store.set` automatically serializes value for storage (and `Store.get` deserializes automatically as well): ```typescript async set(namespace: string, key: string, value: StoredData): Promise<void> { const validated = StoredDataSchema.parse(value); // <platform specific serialization> this.notifyListeners(namespace, key, validated.data); } ``` The legacy approach still works due to how platform-dependent serialization/deserialization handles `string` values specifically. When we call `JSON.stringify(data)` in legacy modules, we're converting an object to a JSON string. When this string is passed to `persistenceService.setLocalConfig()`, it goes through the store implementation where `<platform>.<stringify>()` is used. When `<platform>.<stringify>()` receives a primitive string value (which we've already stringified), it simply treats it as a regular string value and serializes it accordingly. During retrieval, `<platform>.<parse>()` deserializes this back into a string. Basically, this works due to a "wrapping" pattern that the data goes through: ``` Object → JSON.stringify → string → <platform>.<stringify>() → stored string → <platform>.<parse>() → string → JSON.parse → Object ``` This is definitely not ideal: - It's inefficient - We lose type information - There's a chance of forgetting `JSON.stringify` (for legacy method calls) - Creates unnecessary storage bloat This PR introduces new methods `set`, `get` and `remove` in order to incrementally refactor existing code to the correct usage while keeping the codebase stable for backwards compatibility. For future changes, it's important to note that we have migrations in place for certain keys in `PersistenceService`. When possible, add variables to the migration mechanism to make sure data isn't lost midway. Not all changes necessitate migrations - for example, `login_state` can be replaced without a migration as it will simply prompt users to log back in. However, some data is synced with the backend (either selfhosted or cloud): ```typescript platform.sync.settings.initSettingsSync(), platform.sync.collections.initCollectionsSync(), platform.sync.history.initHistorySync(), platform.sync.environments.initEnvironmentsSync(), ``` These synchronized items definitely need migrations to ensure users don't lose their data, and we've provided those migrations in the service. #### Note to Reviewers There still exist sync calls to `PersistenceService` methods, especially in `std/interceptors`, `sh-desktop/platform` and similar legacy modules. Although these modules are not directly in use anymore, there may exist some static function calls that may be used by live modules indirectly, even though they don't depend on the kernel. These modules are up for removal upon ongoing evaluation, so any `PersistenceService` in these modules can be safely ignored. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>