Jump to conversation
Unresolved conversations (19)
@laurent22 laurent22 May 9, 2020
No mutation, so please create a new array `newSecureItems` and then `newSecureItems.push({ key: .... value: ....})`
ReactNativeClient/lib/shim-init-node.js
jyuvaraj03 laurent22
@laurent22 laurent22 May 9, 2020
Why did you need to override this method?
CliClient/tests/models_Setting.js
laurent22 jyuvaraj03
@laurent22 laurent22 Apr 5, 2020
Could you abstract this away and make `shim.loadSecureItems` return an array of `{ key: 'foo', value: 'bar' }`?
Outdated
ReactNativeClient/lib/models/Setting.js
jyuvaraj03
@laurent22 laurent22 Apr 5, 2020
Please make async
Outdated
ReactNativeClient/lib/shim.js
jyuvaraj03
@laurent22 laurent22 Apr 5, 2020
Can be removed.
Outdated
ReactNativeClient/lib/shim.js
@laurent22 laurent22 Apr 5, 2020
As this is only used by your code, please remove it from the shim and keep as a private helper method.
Outdated
ReactNativeClient/lib/shim-init-node.js
jyuvaraj03 laurent22
@laurent22 laurent22 Apr 5, 2020
Please make these methods async. Keytar might be synchronous, but it's likely that for example a React Native lib would be async, so it needs to work with both.
Outdated
ReactNativeClient/lib/shim-init-node.js
@laurent22 laurent22 Apr 5, 2020
Should be `await shim.saveSecureItem(....`
Outdated
ReactNativeClient/lib/models/Setting.js
@laurent22 laurent22 Apr 5, 2020
The logic is not right here because the `load()` function will exit before your code here has run, which could cause race conditions. I think you should change this to `return shim.loadSecureItems(...` although ideally all this method should be refactored with async/await.
Outdated
ReactNativeClient/lib/models/Setting.js
jyuvaraj03
@laurent22 laurent22 Mar 27, 2020
If you refactor the functions as described above you won't need all this duplicated code.
Outdated
ReactNativeClient/lib/shim-init-react.js
jyuvaraj03
@laurent22 laurent22 Mar 27, 2020
Same comment as above. That function should save a secure item and that's it.
Outdated
ReactNativeClient/lib/shim-init-node.js
@laurent22 laurent22 Mar 27, 2020
loadSecureItems should only do the absolute minimum related to keychain - it should return an array with the relevant credentials and that's it. There's shouldn't be Setting related code, let alone dispatching actions, in here. That also means refactoring the call in Setting.js
Outdated
ReactNativeClient/lib/shim-init-node.js
jyuvaraj03
@laurent22 laurent22 Mar 27, 2020
I don't think this tests anything related to this feature, because the test env may or may not have a keychain.
Outdated
CliClient/tests/models_Setting.js
jyuvaraj03
@laurent22 laurent22 Mar 27, 2020
setValue is not async
Outdated
CliClient/tests/models_Setting.js
jyuvaraj03
@laurent22 laurent22 Mar 27, 2020
should be in beforeEach
Outdated
CliClient/tests/models_Setting.js
@laurent22 laurent22 Mar 27, 2020
That's not specific to the keychain feature
Outdated
CliClient/tests/models_Setting.js
@laurent22 laurent22 Mar 27, 2020
`keytar` is installed and always available so no need for a try/catch block
ReactNativeClient/lib/shim-init-node.js
jyuvaraj03 laurent22
@laurent22 laurent22 Mar 23, 2020
Again no device check in Setting.js. Use the shim
Outdated
ReactNativeClient/lib/models/Setting.js
jyuvaraj03
@laurent22 laurent22 Mar 23, 2020
Please don't check for app type here. Instead use lib/shim.js to abstract away differences between platforms. For example create a function in the shim that returns all the secure credentials. On Node/Electron, make it use keytar. On React Native, since it's not implemented, make it return an empty array. Make sure that the logic means that it works both on RN and Node.
Outdated
ReactNativeClient/lib/models/Setting.js
jyuvaraj03
Resolved conversations (0)