chore: a tiny bit safer approach #482
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #482 +/- ##
==========================================
+ Coverage 73.86% 74.40% +0.54%
==========================================
Files 109 110 +1
Lines 10356 10480 +124
Branches 686 706 +20
==========================================
+ Hits 7649 7798 +149
+ Misses 2704 2679 -25
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the safeCopy utility to use Node.js's native fs.copyFile instead of manual readFile/writeFile operations, improving efficiency and handling of concurrent operations. Additionally, it removes the aggressive cleanup step (rm) that was previously deleting the entire assets directory before copying.
Key Changes
- Replaced
readFile/writeFilewith nativecopyFileAPI for better performance and atomicity - Removed the
rmcall that was deleting the entire assets directory before copying - Added comprehensive test coverage for the
safeCopyfunction
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/generators/legacy-html/utils/safeCopy.mjs | Refactored to use copyFile instead of manual read/write operations; updated documentation |
| src/generators/legacy-html/utils/tests/safeCopy.test.mjs | Added comprehensive test suite covering various scenarios |
| src/generators/legacy-html/index.mjs | Removed aggressive rm call that deleted the entire assets directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment was marked as resolved.
This comment was marked as resolved.
|
Whoops! I thought the bot would just make a commit, not open a whole different PR. |
|
cc @aduh95 / @avivkeller / @flakey5 can y'all test if y'all see any issues with this one? I tried: Didn't find any errors. |
|
@avivkeller PR is failing |
Co-authored-by: Antoine du Hamel <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@nodejs/web-infra for reviews |
| */ | ||
| export async function safeCopy(srcDir, targetDir) { | ||
| const files = await readdir(srcDir); | ||
| try { |
There was a problem hiding this comment.
Uhm, why we have a try with a catch doing this? I'd rather simplify this :)
Just do one await copyFile with the readdir and stats and call it a day.
Also, copyFile call with a srcDir and targetDir is wrong. copyFile !== cp
There was a problem hiding this comment.
I was following the try/catch in #482 (comment)
Just do one await copyFile with the readdir and stats and call it a day.
By this you mean revert to the state of the PR at the time of @aduh95's approval (without his suggestion)? Or perhaps I'm not understanding?
This PR removes the removal of files during also the crazy copy of files, deferring the out/ folder cleanup to
doccleanas it probably should. The original intent of thermcommand was to ensure files that do not exist on source anymore and are irrelevant will be removed, but this isn't really an issue and eventually the legacy generator will go away.Simplifed to a simple copyFile call too.