Skip to content
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

feat: add arrays to IndexedStorage typings #1863

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

woutermont
Copy link
Contributor

Putting this here should it be of any value. I added this for the work on UMA, but no longer use it. The typings compile and work with the CRUD operations of the WrappedIndexedStorage, but something goes wrong when using the find features. That code is rather hard to follow, though, so if we want to keep this, maybe you can take a look, @joachimvh?

Signed-off-by: Wouter Termont <woutermont@gmail.com>
@joachimvh
Copy link
Member

The idea behind the interface of the indexed storage is to have something similar to what you would have in a SQL store, with the idea of at some point having an implementation that actually uses SQL. Having arrays as an option would sort of go against that. For multiple values it would be expected to instead have a type that stores the value, and references the type where you would want to have the value. And if you then wanted all values for the parent to use find on the new type, with the id of the parent type.

@woutermont
Copy link
Contributor Author

Hm, alright, then I guess we can just drop this.

The use case I initially wrote this for was to load an array of keys per account from the config with the SeededInitializer. How would you do this then?

@joachimvh
Copy link
Member

The use case I initially wrote this for was to load an array of keys per account from the config with the SeededInitializer. How would you do this then?

Have a Key type with accountId as foreign key, similar to how it is done with pods for example.

@woutermont
Copy link
Contributor Author

Sure, but isn't that placing the burden in the wrong place? The initialiser must then process each array to put it in the storage.

Not a big issue though, since I don't need it right now. We can drop this PR.

@joachimvh
Copy link
Member

You could then have an inbetween KeyManager class or something like that, that takes an array of keys as input together with an accountId. The IndexedStorage is expected to be the very low-level storage, with inbetween classes to provide the "expected" functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants