Repository navigation
Complete mapping as of tough-cookie 2.3.2 - #4
Conversation
| * If no cookie is found then null MUST be passed instead (i.e. not an error). | ||
| */ | ||
| httpOnly?: boolean; | ||
| findCookie(domain: string, path: string, key: string, cb: (error: Error, cookie: Cookie) => any): any; |
There was a problem hiding this comment.
Minor nitpick: Can you add a space after the method names to stay consistent with the rest of the file?
Also, should the return type here be void?
There was a problem hiding this comment.
It shouldn't be void - https://github.com/salesforce/tough-cookie/blob/master/lib/memstore.js#L60
There was a problem hiding this comment.
It's closer to void than any, I'd still write this as the callback being => void and the return of the entire function being void too. And I tend to always write my callbacks in that style too, just a weird preference thing for returning when I definitely don't want anything occurring after an async action to indicate intent. If you really want to keep it accurate, would you like to make it a generic instead?
There was a problem hiding this comment.
Should any be void'ed also in places like
setCookie (cookieOrString: string | Cookie, currentUrl: string, cb: (err: Error, cookie?: Cookie) => any): void;
(original code)
There was a problem hiding this comment.
Yes, I think so. I used to write any over void because I had run into some random issues in the past, but the correct type should have been void from the beginning. Feel free to submit another PR to correct that, in the callback positions I haven't really cared since whatever the user puts is good, but it should still prefer void I believe.
There was a problem hiding this comment.
Updated yet keeping consistent with original code
| * // yuck! | ||
| * } | ||
| * </code></pre> | ||
| */ |
There was a problem hiding this comment.
I can't recall if it's used anywhere else in the file, but would you mind using markdown-style triple backticks over HTML? I'm not sure either are currently used anywhere though.
|
Looks great! 👍 I just have a couple of minor comments and then I'll merge. Also, if you want to update https://github.com/types/npm-tough-cookie/blob/05f8c59af4823af51f3ea54927c43253696bc71f/typings.json#L3 it might be helpful too (just to |
|
@blakeembrey replaced the HTML with markdown. Haven't noticed that those were used already :) |
|
Thanks again 👍 |
2 participants
No description provided.