-
-
Notifications
You must be signed in to change notification settings - Fork 585
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
Initial framework for indexeddb-backed crypto store #445
Conversation
Doesn't do anything useful yet - just demonstrates a framework for how I hope it will fit into the sdk.
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.
Other than JSDoc stuff, LGTM.
@@ -160,6 +160,7 @@ function MatrixClient(opts) { | |||
opts.sessionStore, | |||
userId, this.deviceId, | |||
this.store, | |||
opts.cryptoStore, |
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.
Missing JSDoc in function MatrixClient(opts)
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.
fixed
@@ -63,6 +63,11 @@ module.exports.TimelineWindow = require("./timeline-window").TimelineWindow; | |||
module.exports.InteractiveAuth = require("./interactive-auth"); | |||
|
|||
|
|||
module.exports.MemoryCryptoStore = | |||
require("./crypto/store/memory-crypto-store").default; |
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.
What does .default
do? I thought the default export is the default if you just require()
and use it?
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.
apparently that was true in babel 5, but in babel 6 the default export is given the special default
name in module.exports
. ¯_(ツ)_/¯
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.
¯\_(ツ)_/¯ ok then
src/matrix.js
Outdated
* | ||
* @param {module:crypto.store.base~CryptoStore=} opts.cryptoStore | ||
* crypto store implementation. Calls the factory supplied to | ||
* {@link setCryptoStoreFactory if unspecified} |
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.
End brace mispositioned.
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.
fixed
@@ -136,6 +154,11 @@ module.exports.wrapRequest = function(wrapper) { | |||
* {@link module:scheduler~MatrixScheduler}. | |||
* @param {requestFunction} opts.request If not set, defaults to the function | |||
* supplied to {@link request} which defaults to the request module from NPM. | |||
* | |||
* @param {module:crypto.store.base~CryptoStore=} opts.cryptoStore | |||
* crypto store implementation. Calls the factory supplied to |
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.
Should specify what the default is if the caller never explicitly calls setCryptoStoreFactory
. Does it error out? In-memory?
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.
fixed
/** | ||
* @implements {module:crypto/store/base~CryptoStore} | ||
*/ | ||
export default class IndexedDBCryptoStore { |
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.
This doesn't get displayed correctly when you npm run gendoc
. The class is listed as crypto/store/indexeddb-crypto-store
and it misses the constructor.
It's likely that this is happening because you're writing this in ES6 which JSDoc parses but does so incorrectly, for example members of class instances jsdoc/jsdoc#1301 - this is why we still use old-style ES5 in the JS SDK.
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.
The class is shown as crypto/store/indexeddb-crypto-store
because it's the default export of that module. From an external point of view, its name is not IndexedDBCryptoStore
but rather module:crypto/store/indexeddb-crypto-store
.
It's missing the constructor because I forgot to document it. Now fixed.
jsdoc mostly.
LGTM! |
thanks. I'm going to put the store-clearing stuff in here too in a sec. |
- to clear both sets of storage on logout
LGTM |
Doesn't do anything useful yet - just demonstrates a framework for how I hope it will fit into the sdk.
The idea here is that you can configure a factory to build a crypto store whenever you call
createClient
(if you don't pass one in explicitly). The default is an in-memory store, butbrowser-index.js
sets the factory to the indexeddb impl.