fix: move to async functions where possible #106
Conversation
|
Linting is failing until npm/template-oss#141 lands. There was a test that was enforcing this, moved it to a linting rule but now the main eslintrc.js file is failing that very rule. Irony. ETA: this has been done |
| return lstat(cpath).then((stat) => ({ stat, cpath, sri })) | ||
| }).then(({ stat, cpath, sri }) => { | ||
| // Set all this up to run on the stream and then just return the stream | ||
| Promise.resolve().then(async () => { |
There was a problem hiding this comment.
it took me a second to see what this was doing.. seems like the intent here is to run the code within the .then() handler after a short delay?
i don't have a solid suggestion for making this more readable, setImmediate(async () => {}) feels like it would be, but you'd have to also put a try/catch inside the function. maybe just a comment explaining is enough?
There was a problem hiding this comment.
Yeah I fiddled a bit w/ this, this was as good as it got. The comment was supposed to explain it lol.
| const content = await hasContent(cache, integrity) | ||
| // ~pretty~ sure we can't end up with a content lacking sri, but be safe | ||
| if (content && content.sri) { | ||
| await rimraf(contentPath(cache, content.sri)) |
There was a problem hiding this comment.
since we've brought @npmcli/fs in already, this could be fs.rm() instead of rimraf and the rimraf dependency can go away (after checking for other uses, of course)
There was a problem hiding this comment.
just did more review and found that we use rimraf.sync in at least one place, so the above would actually require removing the synchronous interface from cacache. i'm personally in favor of doing so, but it should very much be a separate pull request
There was a problem hiding this comment.
Yeah this was ONLY supposed to move to @npmcli/fs. Switching to its functionality specifically was always intented to be a separate PR.
| } | ||
|
|
||
| return Promise.resolve(inferOwner(cache)).then((owner) => { | ||
| const { uid, gid } = owner | ||
| const { uid, gid } = await inferOwner(cache) |
There was a problem hiding this comment.
the infer-owner dependency here can also leverage @npmcli/fs in a future refactor pass
This refactors the code to use async functions where possible. It also uses `@npmcli/fs` consistently (it was already a dependency, just not used). It also reorders the test files to match their sources in `./lib`. Tests were not refactored (except where needed to move to `@npmcli/fs`) in the interest of an easier PR review.
This refactors the code to use async functions where possible.
It also uses
@npmcli/fsconsistently (it was already a dependency,just not used).
It also reorders the test files to match their sources in
./lib.Tests were not refactored (except where needed to move to
@npmcli/fs)in the interest of an easier PR review.