feat(web): add findStores functions 🍪 🎼
#15437
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change adds a static `findStores` function to `VarStoreSerializer` and `VariableStoreCookieSerializer` as well as to the `VariableStoreSerializer` interface. Also add unit tests for `VariableStoreCookieSerializer`. Part-of: #13426 Test-bot: skip
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
keymanapp-test-bot
bot
changed the title
findStores functionsfindStores functions 🎼
ermshiperete
changed the title
findStores functions 🎼findStores functions 🍪 🎼
mcdurdin
approved these changes
Jan 18, 2026
mcdurdin
reviewed
Jan 18, 2026
| @@ -9,27 +9,51 @@ import { CookieSerializer } from "keyman/engine/dom-utils"; | |||
| // dynamic property names; they'd have to be known at compile time to facilitate | |||
| // strict type checking. | |||
| class VarStoreSerializer extends CookieSerializer<VariableStore> { | |||
Member
There was a problem hiding this comment.
So ... we have VarStoreSerializer, VariableStoreSerializer, VariableStoreCookieSerializer, and CookieSerializer<VariableStore>. Yuck! Why do we need 4? Can't we do this all with just one? What benefit are we getting with this complexity and confusion?
Contributor
Author
There was a problem hiding this comment.
Follow-up in #15470. It reduces the number of types, although it's still more than one - see PR description.
mcdurdin
reviewed
Jan 18, 2026
Comment on lines
9
to
10
| // dynamic property names; they'd have to be known at compile time to facilitate | ||
| // strict type checking. |
Member
There was a problem hiding this comment.
This comment (lines 4-10) is an excuse for the implementation, but doesn't help describe the purpose of the class. It's missing background which doesn't help a reader understand (a) what is this 'transformation' for, etc.
Contributor
Author
There was a problem hiding this comment.
I left that comment for now.
ermshiperete
added a commit
that referenced
this pull request
Jan 19, 2026
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>` - also fix baseline tests to use dynamic time value and set cookies to expire in 60s instead of at some arbitrary time which might be in the past. Follow-up-of: #15437 Test-bot: skip
ermshiperete
added a commit
that referenced
this pull request
Jan 19, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change adds a static
findStoresfunction toVarStoreSerializerandVariableStoreCookieSerializeras well as to theVariableStoreSerializerinterface.Also add unit tests for
VariableStoreCookieSerializer.Part-of: #13426
Test-bot: skip