Skip to content

[Refactor] Simplify copyDirectoryContents implementation#7433

Open
gonzaloriestra wants to merge 1 commit intomainfrom
refactor-copy-directory-contents-10066322472617573812
Open

[Refactor] Simplify copyDirectoryContents implementation#7433
gonzaloriestra wants to merge 1 commit intomainfrom
refactor-copy-directory-contents-10066322472617573812

Conversation

@gonzaloriestra
Copy link
Copy Markdown
Contributor

@gonzaloriestra gonzaloriestra commented Apr 30, 2026

WHY are these changes introduced?

copyDirectoryContents in packages/cli-kit/src/public/node/fs.ts reimplemented directory copying by hand: it created the destination, ran a glob over the source tree, and fanned out one copyFile per match through Promise.all. That extra logic is harder to reason about and can hit EMFILE ("too many open files") on large trees because every file is opened concurrently. fs-extra's fsCopy already handles directory creation, recursion, and back-pressure correctly.

WHAT is this pull request doing?

  • Replaces the manual glob + Promise.all/copyFile loop in copyDirectoryContents with a single fsCopy(srcDir, destDir) call.
  • Keeps the existing source-existence check so callers still get the same error message when the source directory is missing.
  • Removes the redundant mkdir for the destination (handled by fsCopy).

How to test your changes?

copyDirectoryContents is exercised by the include_assets build step for UI extensions when the source is a directory. Build any app whose extension declares an include_assets entry pointing at a directory and confirm the contents are copied recursively:

pnpm install
pnpm shopify app build --path <path-to-app>

After the build, verify under the extension's output directory (<path-to-app>/dist/<extension>/...) that:

  • the target directory exists,
  • all nested files and subdirectories from the source are present (counts and contents match the source),
  • the build completes without EMFILE / ENOENT errors.

Negative path — make sure the original error message is preserved when the source is missing. Run from the repo root against the built bundle:

pnpm nx build cli-kit
node -e "import('./packages/cli-kit/dist/public/node/fs.js').then(({copyDirectoryContents}) => copyDirectoryContents('/tmp/does-not-exist-xyz', '/tmp/dest').catch((err) => console.log(err.message)))"
# Expect: Source directory /tmp/does-not-exist-xyz does not exist

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact
  • The change is user-facing — I've identified the correct bump type (patch for bug fixes · minor for new features · major for breaking changes) and added a changeset with pnpm changeset add

@google-labs-jules
Copy link
Copy Markdown

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@gonzaloriestra gonzaloriestra requested a review from a team as a code owner April 30, 2026 00:29
@github-actions github-actions Bot added cla-needed no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users. labels Apr 30, 2026
Replaced manual glob and loop with a single fsCopy call from fs-extra.
This improves performance, is more robust against EMFILE errors on
large directories, and significantly simplifies the code while
maintaining the same behavior.

Made-with: Cursor
@gonzaloriestra gonzaloriestra force-pushed the refactor-copy-directory-contents-10066322472617573812 branch from e8d9c22 to dad8554 Compare April 30, 2026 07:59
@gonzaloriestra gonzaloriestra changed the title 🧹 Refactor: simplify copyDirectoryContents implementation [Refactor] Simplify copyDirectoryContents implementation Apr 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant