return promise if callback not passed to async methods #157
Conversation
|
@jstewmon regarding vows: yeah, that seemed like a good idea at the time. I agree literally anything is better than vows ;) |
jstewmon
force-pushed
the
promises
branch
2 times, most recently
from
58daa88 to
3ab06cf
Compare
|
@medelibero-sfdc 👋 , I'm working on rebasing #156 now. Let's please make this the last time I have to rebase - #156 has so many line moves that it is very tedious to integrate long runs of changes from master. 🙏 |
|
@medelibero-sfdc unfortunately, #173 precludes a clean path forward here because the callback should always be the last argument... Also, that PR did not update the interface in the Store base class. I have addressed those two issues in #156 in a new commit that notes the breaking change. |
|
Thank you for your help with this @jstewmon; sorry to ask for so many rebases. We should be good going forward. |
| if (!this.idx[domain]) { | ||
| return cb(null,undefined); | ||
| "use strict"; | ||
| const { fromCallback } = require("universalify"); |
There was a problem hiding this comment.
If we move to more modern syntax; won't this effect our dependents? I think if we are targeting older platforms, we may need to talk more about this.
There was a problem hiding this comment.
Destructuring assignment is supported in node 6+.
lib/memstore.js
Outdated
| Object.keys(domainIndex).forEach(cookiePath => { | ||
| if (pathMatch(path, cookiePath)) { | ||
| const pathIndex = domainIndex[cookiePath]; | ||
|
|
There was a problem hiding this comment.
Let's drop the whitespace here.
awaterma
requested review from
awaterma
and removed request for
medelibero-sfdc
jstewmon
force-pushed
the
promises
branch
2 times, most recently
from
61a1126 to
e0f7777
Compare
|
@awaterma thanks for the review. I rebased on master and addressed your feedback. |
|
👋 @awaterma freshly rebased if you have time to take another look 🙏 |
|
I think this looks good; I'd like to discuss a bit with the open source term before merging. I'd rather not add an additional dependency; but "universalify" does provide the right solution. It's more of an extra dependency trade off discussion. :) Anyway, we should have good progress on this soon. |
There was a problem hiding this comment.
We've discussed this at length and are ready to pull it in; there a small merge with master that needs to be done before, however. I can do that through the console, or we can wait on @jstewmon to update. I'll wait a day for a reply and if we don't hear back, I'll try merging master into this branch with GitHub; then approve the final p/r.
|
@awaterma thanks for pushing this over the line! 🙏 🎉 Sorry for not responding over the last week - just been exceptionally busy at work. 😅 |
|
We're grateful for the help! Thank you for contributing! |
This is based on #155 and #156 . It could use a few more tests, but I wanted to get some feedback before writing any more.
BTW, vows seems dead, and has terrible ergonomics for async tests. Any interest in switching to something else? I'm partial to ava these days, but literally anything would be better than vows. :-)