feat(web): cleanup of variable stores 🎼 #15470

ermshiperete · 2026-01-19T15:16:40Z

This addresses code review comments in #15437 and removes some unnecessary definitions related to variable stores. It keeps the VariableStoreSerializer interface although currently VariableStoreCookieSerializer is the only implementation. Conceptually IMO it makes sense to use a more abstract interface so that in the future for example we could easily use a different store for a node implementation.

Details of this change:

remove VariableStoreDictionary interface and replace with VariableStore type
remove VarStoreSerializer class and replace with CookieSerializer<VariableStore>

Follow-up-of: #15437
Test-bot: skip

keymanapp-test-bot · 2026-01-19T15:17:01Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Android
- Keyman for Android apk - build : all tests passed (no artifacts on BuildLevel "build")
- FirstVoices Keyboards for Android apk - build : all tests passed (no artifacts on BuildLevel "build")
- FirstVoices Keyboards for Android apk (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
- KeyboardHarness apk - build : all tests passed (no artifacts on BuildLevel "build")
- Keyman for Android apk (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
- KMSample1 apk - build : all tests passed (no artifacts on BuildLevel "build")
- KMSample2 apk - build : all tests passed (no artifacts on BuildLevel "build")
Developer
- Keyman Developer - build : all tests passed (no artifacts on BuildLevel "build")
- Compiler Regression Tests - build : all tests passed (no artifacts on BuildLevel "build")
- Keyman Developer (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
- kmcomp.zip - build : all tests passed (no artifacts on BuildLevel "build")
- kmcomp.zip (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
iOS
- Keyman for iOS (simulator image) - build : all tests passed (no artifacts on BuildLevel "build")
- FirstVoices Keyboards for iOS (simulator image) - build : all tests passed (no artifacts on BuildLevel "build")
- FirstVoices Keyboards for iOS (simulator image) (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
- Keyman for iOS (simulator image) (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
Keyboards
- Test Keyboards - build : all tests passed (no artifacts on BuildLevel "build")
Web
- KeymanWeb Test Home - build : all tests passed (no artifacts on BuildLevel "build")

This addresses code review comments in #15437 and removes some unnecessary definitions related to variable stores. It keeps the `VariableStoreSerializer` interface although currently `VariableStoreCookieSerializer` is the only implementation. Conceptually IMO it makes sense to use a more abstract interface so that in the future for example we could easily use a different store for a node implementation. Details of this change: - remove `VariableStoreDictionary` interface and replace with `VariableStore` type - remove `VarStoreSerializer` class and replace with ` CookieSerializer<VariableStore>` Follow-up-of: #15437 Test-bot: skip

mcdurdin · 2026-01-19T19:26:17Z

web/src/engine/src/keyboard/keyboards/jsKeyboard.ts

   * @returns an object with each property referencing a variable store
   */
-  get variableStores(): VariableStoreDictionary {
+  get variableStores(): VariableStore {


hmm, the function is plural but the return value is singular. Is that right?

Changed to plural.

mcdurdin · 2026-01-19T19:27:50Z

web/src/engine/src/keyboard/variableStore.ts

-  [name: string]: string;
-};
-
 export type VariableStore = { [name: string]: string; };


This should probably be VariableStores since it is can contain more than one value.

See comment below (#15470 (comment)).

mcdurdin · 2026-01-20T21:09:56Z

web/src/engine/src/keyboard/variableStores.ts

+  loadStore(keyboardID: string, storeName: string): VariableStores;
+  saveStore(keyboardID: string, storeName: string, storeMap: VariableStores): void;
+  findStores(keyboardID: string): VariableStores[];


This doesn't make sense to me. loadStore should be returning 1 store. findStores shouldn't be finding an array of a set of variable stores!

It's not as bad as it looks (other than that it looks bad and might be confusing): currently each VariableStores object contains only one key/value pair (except in tests), so loadStore and saveStore function names make sense. findStores returns all cookies/stores for a keyboard.

Thinking about it - maybe the term store is part of a problem. In every day life I think of a store as something where you can get different things. With that in mind, a VariableStore in our code would be able to hold multiple key/value pairs. The plural VariableStores would be several stores, each holding multiple key/value pairs.

But if we think from .kmn, store(x) defines a variable store, i.e. one key/value pair. This calls for naming the type that can hold multiple key/value pairs a VariableStores type.

Since currently we store only one key/value pair in that type although it can hold multiple ones, I think it's probably best to change it back to VariableStore and use it in the sense of a .kmn variable store.

I think if we implement what's discussed in #15436 (comment) things might get a bit cleaner. It doesn't help with the terminology, but at least then it would hold only one key/value pair and the type would match that.

If we only ever store one key + value in the object then why don't we change it to a more conventional pattern:

export type VariableStore = { name: string; value: string; }

That also then allows us to add extra metadata in the future if we want and eliminates this confusion. IMHO the current model is fragile and liable to trip maintainers up in the future (as we can see in this discussion where we are going around in circles). I can't see that we are deriving any benefits from the current model, either in performance or in clarity.

Either that, or we refactor to store all of the variable stores in a single object... but that seems more complicated from my reading of the code.

(except in tests)

If tests are doing something different to the real-life use of the code, then the tests are definitely wrong -- tests must match the real-life usage or they are testing the wrong thing. IMVHO it's crazy that the only place this pattern is used is in the unit tests!

I'm not entirely sure that my analysis was correct.

jsKeyboard.ts has this code: values[store] = this.scriptObject[store]; and I'm not sure what scriptObject[store] can return.

Yes, that is certainly adding more than one value into the VariableStore type. From my current reading, it's buggy -- in one place we add multiple values to one VariableStore but in other places we have multiple VariableStores. I think we need to review again -- it's a very simple data model, but the implementation seems overly complicated right now.

ermshiperete · 2026-01-21T11:40:07Z

Reverted the rename from VariableStore → VariableStores based on #15470 (comment).

mcdurdin · 2026-02-01T18:43:22Z

web/src/engine/src/main/variableStoreCookieSerializer.ts

@@ -54,6 +32,8 @@ export class VariableStoreCookieSerializer implements VariableStoreSerializer {
   * @returns An array of VariableStore objects found for the keyboard.
   */
  public findStores(keyboardID: string): VariableStore[] {


This is the broken function, right? VariableStore is already a map, so it should be returning a map, not an array of maps. And this is a change in epic/web-core, not in the current alpha, so I think it's a change you made based on VariableStore being badly named.

Should we go ahead with naming it VariableStores and eliminate VariableStoreDictionary?

ermshiperete requested a review from mcdurdin January 19, 2026 15:16

github-project-automation bot added this to Keyman Jan 19, 2026

github-project-automation bot moved this to Todo in Keyman Jan 19, 2026

github-actions bot added web/ feat labels Jan 19, 2026

keymanapp-test-bot bot added the epic-web-core label Jan 19, 2026

keymanapp-test-bot bot changed the title ~~feat(web): cleanup of variable stores~~ feat(web): cleanup of variable stores 🎼 Jan 19, 2026

keymanapp-test-bot bot added this to the A19S21 milestone Jan 19, 2026

This was referenced Jan 19, 2026

feat(web): add findStores functions 🍪 🎼 #15437

Merged

refactor(web): rename DefaultRules → DefaultOutputRules 📏 🎼 #15464

Merged

feat(core): expose km_core_state_options_update to WASM 🍪 🎼 #15438

Merged

ermshiperete force-pushed the feat/web/varStoresCleanup branch from 8fb8c95 to 01bee77 Compare January 19, 2026 17:18

ermshiperete changed the base branch from epic/web-core to fix/web/baselinetests January 19, 2026 17:20

mcdurdin requested changes Jan 19, 2026

View reviewed changes

Base automatically changed from fix/web/baselinetests to epic/web-core January 20, 2026 07:35

ermshiperete requested a review from mcdurdin January 20, 2026 14:08

mcdurdin requested changes Jan 20, 2026

View reviewed changes

ermshiperete force-pushed the feat/web/varStoresCleanup branch from b94661a to 01bee77 Compare January 21, 2026 11:38

ermshiperete requested a review from mcdurdin January 21, 2026 11:40

keyman-server modified the milestones: A19S21, A19S22 Jan 31, 2026

mcdurdin reviewed Feb 1, 2026

View reviewed changes

Uh oh!

feat(web): cleanup of variable stores 🎼 #15470

Are you sure you want to change the base?

feat(web): cleanup of variable stores 🎼 #15470

Conversation

ermshiperete commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keymanapp-test-bot bot commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ermshiperete Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ermshiperete Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ermshiperete commented Jan 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ermshiperete commented Jan 19, 2026 •

edited

Loading

keymanapp-test-bot bot commented Jan 19, 2026 •

edited

Loading

ermshiperete Jan 21, 2026 •

edited

Loading

ermshiperete Jan 27, 2026 •

edited

Loading