-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add Key value store based on a typescript port of the keyv package #1150
Add Key value store based on a typescript port of the keyv package #1150
Conversation
…age. Includes TypeORM, Map based and Tiered Key Value storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this plugin is introducing top level methods, it has to have a plugin.schema.json
file which is going to be used when generating OpenAPI schema for remote method execution. This means that the interface and arguments of these methods have to be serialisable.
You have to add this section to the package.json
file:
"veramo": {
"pluginInterfaces": {
"IKeyValueStore": "./src/key-value-types.ts"
}
},
and add this line to the scripts
section:
"generate-plugin-schema": "node ../cli/bin/veramo.js dev generate-plugin-schema"
When you do this, you will see that this script is complaining about the types. I think the issue is that you are using <ValueType>
in interface definitions. I don't think this is supported by the tools we are using for schema generation.
# Conflicts: # pnpm-lock.yaml
…lacing any '>' character. Now only applies to typed arrays
Hi @simonas-notcat Thanks for the review and sorry for not getting back sooner. It has been a bit hectic over here. I am not sure about your plugin/schema comment. This is not a Veramo plugin to be clear. It is more like the key manager, or even better the data-store. Both which are not exposing a plugin schema. It is not a plugin, because you might want to use one or more KV stores in your own plugins. Then you want to have typed values for your keys per KV store. You are not going to call a KV Veramo instance remotely, you would use a remote backing store, like a TypeORM DB or Mongo for instance. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## next #1150 +/- ##
==========================================
+ Coverage 80.25% 83.60% +3.35%
==========================================
Files 118 167 +49
Lines 4056 16989 +12933
Branches 875 1849 +974
==========================================
+ Hits 3255 14204 +10949
- Misses 798 2785 +1987
+ Partials 3 0 -3
☔ View full report in Codecov by Sentry. |
Sorry I did not realise that. Now it makes sense. Looks good to me |
@mirceanis Do you also want to do a review or chime in on the PR, given you had some opinions/suggestions in Discord as well? |
@mirceanis Nudge, nudge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! There are just a few nits.
Co-authored-by: Mircea Nistor <[email protected]>
Co-authored-by: Mircea Nistor <[email protected]>
Rever change outside of kv-store from refactoring
@@ -31,7 +31,7 @@ function createSchema(generator: TJS.SchemaGenerator, symbol: string) { | |||
return { components: { schemas: {} } } | |||
} | |||
|
|||
let fixedSymbol = symbol.replace('Array<', '').replace('>', '') | |||
let fixedSymbol = symbol.replace(/Array\<(.*)\>/gm, '$1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mirceanis Just wanted to highlight this change outside of the kv-store package. When testing a bit with the schema-generator I noticed that it mangled anything with carets in there, whilst it was only intended for Arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did notice this, thanks for adding a more elegant solution!
Okay comments are addressed. 2 things:
|
That's exactly how it should be :) |
This is a Key Value store with a typescript port of the Keyv package (https://github.com/jaredwray/keyv)., ESM compatible and should work in browser/Node/React-Native (not tested yet)
Includes TypeORM, Map based and Tiered Key Value storage adapters.
Lastly there is a separate API/Interfaces within Veramo, to not expose core data types from the package.
Tests added and code "fully" implemented.
Happy to get some feedback