Refactor copy API in async/await
#1021
Conversation
SukkaW
force-pushed
the
promisfy-fs-extra-1
branch
from
4fe8f1d to
f04093f
Compare
SukkaW
changed the title
copy API in async/awaitcopy API in async/await
|
@RyanZim The PR is ready to be reviewed now. |
SukkaW
force-pushed
the
promisfy-fs-extra-1
branch
from
a948dc0 to
d9f8ef9
Compare
| const path = require('path') | ||
| const assert = require('assert') | ||
| const copy = require('../copy') | ||
| const { copy } = require('../') |
There was a problem hiding this comment.
I need to change the import here. lib/copy/copy.js now exports the promise-only API (instead of the callback-only API), while lib/copy/index.js now exports the unified API.
| // reset stubs | ||
| gracefulFsStub = {} | ||
| utimes = proxyquire('../utimes', { 'graceful-fs': gracefulFsStub }) | ||
| utimes = proxyquire('../utimes', { '../fs': gracefulFsStub }) |
There was a problem hiding this comment.
The mock here has to be replaced, as lib/util/utimes.js no longer uses graceful-fs directly. It now uses the unified API from lib/fs/index.js.
RyanZim
left a comment
RyanZim
left a comment
There was a problem hiding this comment.
I've left a several code comments. Additionally, I have a few other concerns:
copyis not expected to return a value, yet we usereturnthroughout the code for final async calls inside functions; this could result in unintentionally returning a value in the future; perhaps we should useawaitinstead?- Is there a reason for duplicating the util functions vs. just wrapping them in universalify? We could wrap them in
fromCallbackhere, then port them to promise implementations and wrap infromPromisein a separate PR. Anything I'm missing? (I have not reviewed those changes yet)
SukkaW
force-pushed
the
promisfy-fs-extra-1
branch
from
e4b7aa1 to
bc8ee0b
Compare
| const fakeFd = Math.random() | ||
|
|
||
| gracefulFsStub.open = (pathIgnored, flagsIgnored, modeIgnored, callback) => { | ||
| gracefulFsStub.open = u((pathIgnored, flagsIgnored, modeIgnored, callback) => { |
There was a problem hiding this comment.
The gracefulFsStub needs to be universalify as the utimesMillis is now Promise-based and then universalized.
|
@RyanZim I've addressed all the code review suggestions. I've also universalized the related internal utility functions to reduce the duplicated implementations. The CI still passes after the latest changes: https://github.com/SukkaW/node-fs-extra/actions/runs/6598480115 Note that the |
2 participants
The PR is the first part of proposal #1020:
copyAPI. Other APIs will be migrated in the following PRs