refactor: use node:fs/promises #11206
Conversation
alumni
force-pushed
the
improvement/fs-promises
branch
2 times, most recently
from
c8dfe76 to
88a3d93
Compare
| @@ -1,5 +1,6 @@ | |||
| import "reflect-metadata" | |||
There was a problem hiding this comment.
Why did you remove reflect metadata here? Is it really not necessary?
There was a problem hiding this comment.
Not necessary. reflect-metadata only needs importing once.
There was a problem hiding this comment.
If test/utils/test-setup.ts is loaded correctly by mocha before the tests, this should definitely not be needed. I noticed some files also load test-setup.ts by themselves, this should also not be required.
It looks like a bigger cleanup is required for the test setup, so I reverted this for now.
| await promisify(unlink)(logPath) | ||
| } | ||
| try { | ||
| fs.unlink(logPath) |
There was a problem hiding this comment.
This was a good catch, I somehow didn't see it.
There's an ESLint rule for this: @typescript-eslint/no-floating-promises, and it's in the recommended-type-checked config. We should consider enabling these rules.
src/commands/CommandUtils.ts
Outdated
| err ? fail(err) : ok(data.toString()), | ||
| ) | ||
| }) | ||
| return (await fs.readFile(filePath)).toString() |
There was a problem hiding this comment.
Split this into 2 lines
There was a problem hiding this comment.
Split how into two lines?
This is the job of the formatter, this file was already formatted.
Later edit: no matter how I try to split, prettier reverts it to a single line.
There was a problem hiding this comment.
const file = await fs.readFile(filePath))
return file.toString()
src/commands/CommandUtils.ts
Outdated
| @@ -67,7 +66,7 @@ export class CommandUtils { | |||
| * Creates directories recursively. | |||
| */ | |||
| static createDirectories(directory: string) { | |||
There was a problem hiding this comment.
Updaye return type to Promise
src/util/ImportUtils.ts
Outdated
| }) | ||
| try { | ||
| const stats = await fs.stat(potentialPackageJson) | ||
| if (stats.isFile()) { |
There was a problem hiding this comment.
Use an early return here for better readability.
if (!stats.isFile()) {
continue
}
| } | ||
| } catch { |
There was a problem hiding this comment.
What are you catching here? A potential throw from fs.stats?
There was a problem hiding this comment.
Exactly. It's the same thing that was being caught before: https://nodejs.org/api/errors.html#common-system-errors
| @@ -41,7 +38,7 @@ describe("ImportUtils.importOrRequireFile", () => { | |||
| expect(exports.default).to.be.a("function") | |||
| expect(exports.number).to.be.eq(6) | |||
|
|
|||
| rmdirSync(testDir) | |||
| await fs.rmdir(testDir, { recursive: true }) | |||
There was a problem hiding this comment.
Indentation is correct. The file was formatted before commit.
alumni
force-pushed
the
improvement/fs-promises
branch
2 times, most recently
from
b88acce to
c827e75
Compare
alumni
force-pushed
the
improvement/fs-promises
branch
from
c827e75 to
b7cc21b
Compare
Description of change
Replacing the sync functions from
node:fswith async functions fromnode:fs/promises. This should simplify the code by not requiring Promise wrappers and it should not block the event loop anymore.Pull-Request Checklist
masterbranchnpm run formatto apply prettier formattingnpm run testpasses with this changeFixes #0000