Modernize keytar #63
Modernize keytar #63
Conversation
(cherry picked from commit 8efe752)
🔥 add/replacePassword and use setPassword
|
This is great! Didn't look too far into the code - but is there any reason why you're using a worker over libsecret's async API? |
|
@TheSBros Thanks! Primarily because the async bridge wraps the API calls on all three platforms, so it was easiest to use the sync versions for all and throw them on the libuv thread pool. |
|
Nice |
Good catch, I've made this change. |
There was a problem hiding this comment.
I left some minor comments, but overall this looks good to me. Amazing job, @kuychaco and @BinaryMuse! ✨
|
|
||
| * Debian/Ubuntu: `sudo apt-get install libsecret-1-dev` | ||
| * Red Hat-based: `sudo yum install libsecret-devel` | ||
| * Arch Linux: `sudo pacman -S libsecret` |
There was a problem hiding this comment.
Do you think we will need to update the Debian and RPM package manifests on Atom too?
There was a problem hiding this comment.
(I made the libsecret patch) The deb/rpm packages will need the non-dev dependencies, which I believe are libsecret-1-0 and libsecret respectively.
lib/keytar.js
Outdated
| checkRequired(service, 'Service') | ||
| checkRequired(account, 'Account') | ||
|
|
||
| var ret = callbackOrPromise(callback) |
There was a problem hiding this comment.
Is there any reason why we need to support both async styles? If not, could we expose only the Promise-based API?
There was a problem hiding this comment.
Good question. Callbacks are Node's "lowest common denominator" for async APIs, so it seemed reasonable to include it. But some browser APIs are going Promise-based. I am easily swayed either direction on this one.
There was a problem hiding this comment.
Yeah, I think I am ok with either too honestly. Maybe, given that we are going to use these new methods via the Promise based API, we could just expose that and save an object allocation?
spec/keytar-spec.js
Outdated
| }) | ||
| }) | ||
|
|
||
| it("yeilds null when the password was not found", function() { |
There was a problem hiding this comment.
Minor suggestion: s/yeilds/yields.
| @@ -0,0 +1,130 @@ | |||
| var assert = require('chai').assert | |||
There was a problem hiding this comment.
What do you think about using async/await to increase this test's readability? We could include babel only in specs but still take advantage of the nicer syntax.
There was a problem hiding this comment.
I thought about it but it seemed a pretty hefty price for a small test suite. If you think it'd be worth it I'm happy to pull it in.
There was a problem hiding this comment.
Yeah, it's quite minor honestly. Given we'd pay for it only in tests, I would still suggest doing it so that we can get rid of async.waterfall and use a modern syntax across the entire Atom org.
README.md
Outdated
| A native Node module to get, add, replace, and delete passwords in system's | ||
| keychain. On OS X the passwords are managed by the Keychain, on Linux they are | ||
| managed by Gnome Keyring and on Windows they are managed by Credential Vault. | ||
| A native Node module to get, add, replace, and delete passwords in system's keychain. On OS X the passwords are managed by the Keychain, on Linux they are managed by the Secret Service API/libsecret, and on Windows they are managed by Credential Vault. |
🤘 This looks great. Thanks so much! |
|
Congratulations! I'll be sure to tell some package maintainers I know that they can remove the dependency on gnome-keyring :) |
|
Oh my gosh, yay, thank you! |
|
Any examples of how to use the callback API? EDIT:
|
|
@mritchie712 The callback API was removed in a later commit in this PR (c4fe6de), so the API only returns promises now. |
This pull requests makes some significant changes to keytar to bring it more in line with other modules maintained by the Atom and Electron teams, and would result in a major version bump:
nan🚧 WIP 🚧
Welcome 💭 s if anyone has any.
Closes #60
Closes #57
Closes #53
Closes #47
Closes #46
Closes #58
Closes #17
Closes #7
/cc - FYI in particular @paulcbetts, @as-cii, @kevinsawicki