Skip to content

[Fix] Move scaffolded project to its final directory before installing deps in app init#7600

Open
craigmichaelmartin wants to merge 2 commits into
mainfrom
fix-init-install-in-place
Open

[Fix] Move scaffolded project to its final directory before installing deps in app init#7600
craigmichaelmartin wants to merge 2 commits into
mainfrom
fix-init-install-in-place

Conversation

@craigmichaelmartin
Copy link
Copy Markdown
Contributor

WHY are these changes introduced?

Cross-OS QA on CLI nightly 0.0.0-nightly-20260521132110 surfaced this as a release blocker on Windows + pnpm: after app init --package-manager=pnpm, every node_modules symlink was dangling and users had to manually rm -rf node_modules && pnpm install before doing anything.

Root cause is the install→move ordering in packages/app/src/cli/services/init/init.ts. The whole flow ran inside inTemporaryDirectory: download template → liquid render → install deps → cleanup → git init, then moveFile(templateScaffoldDir, outputDirectory). pnpm on Windows uses NTFS junctions for the virtual store under node_modules/.pnpm/*, and junctions store absolute paths — moving the tree from %TEMP%\…\app to the destination orphans every link.

What is this pull request doing?

In packages/app/src/cli/services/init/init.ts, insert a new "Preparing project directory" task into the existing task list that does ensureAppDirectoryIsAvailable + moveFile(templateScaffoldDir → outputDirectory). Install, cleanup, and git-init then run against outputDirectory instead of the temp dir.

Pre-install steps (template download, liquid render, package.json edits, .gitignore/.npmrc tweaks) still run in the temp dir, so a failure there leaves no half-baked project on disk. The earlier inTemporaryDirectory block still owns cleanup of the (now mostly-empty) temp dir.

Tradeoff worth flagging: a failure during the install task now leaves the scaffolded project at outputDirectory without a populated node_modules. Before, an install failure would just bail and leave nothing behind. I think this is the better tradeoff — users can re-run pnpm install themselves rather than restarting the whole init flow — but happy to add on-failure cleanup if reviewers prefer the old behavior.

How to test your changes?

On a Windows machine with pnpm installed:

  1. Check out this branch and build the CLI (pnpm build).
  2. node packages/cli/bin/dev.js app init --package-manager=pnpm --name=test-app --template=https://github.com/Shopify/shopify-app-template-react-router#main-cli (or use pnpm create-app).
  3. cd test-app && pnpm dev — should run without "module not found" errors. Without this fix it would die immediately because of dangling junctions under node_modules/.pnpm/.

Also worth a smoke test on macOS / Linux that init still produces a working project — same task list, just with the move task inserted earlier.

Local test runs done on this branch:

  • vitest run packages/app/src/cli/services/init/ — 28/28 pass.
  • vitest run packages/app/src/cli/commands/app/init.test packages/app/src/cli/prompts/init/ — 12/12 pass.
  • tsc --noEmit on @shopify/app — clean.
  • eslint src/cli/services/init/init.ts — clean.

Measuring impact

How do we know this change is successful? Choose one:

  • Existing analytics will cater for this addition
  • PR includes analytics changes (Pending Data Discovery Review)
  • Other (please describe)

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • If introducing a user-facing change, I've added a changeset

(A .changeset/fix-init-install-in-place.md is included.)

@github-actions github-actions Bot added the Area: @shopify/app @shopify/app package issues label May 21, 2026
@craigmichaelmartin craigmichaelmartin marked this pull request as ready for review May 21, 2026 19:00
@craigmichaelmartin craigmichaelmartin requested review from a team as code owners May 21, 2026 19:00
@craigmichaelmartin
Copy link
Copy Markdown
Contributor Author

/snapit

@github-actions
Copy link
Copy Markdown
Contributor

🫰✨ Thanks @craigmichaelmartin! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260521190126

Caution

After installing, validate the version by running shopify version in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

…g deps in `app init`

Run the dependency install, cleanup, and git-init steps of `shopify app
init` against the final output directory instead of a temporary scaffold
that is moved into place afterwards. pnpm (and similarly affected
package managers) create absolute-path junctions/symlinks on Windows for
their virtual store, so installing in the temp dir and then moving the
tree orphans every link under `node_modules/.pnpm/*`, forcing every
Windows + pnpm user to manually `rm -rf node_modules && pnpm install`
before doing anything with the new project.

Pre-install steps (template download, liquid render, package.json edits,
.gitignore/.npmrc tweaks) still run in the temp directory so a failure
there leaves no half-baked project on disk.
@gonzaloriestra gonzaloriestra force-pushed the fix-init-install-in-place branch from a565a89 to 45b1884 Compare May 22, 2026 07:59
await ensureAppDirectoryIsAvailable(outputDirectory, hyphenizedName)
await moveFile(templateScaffoldDir, outputDirectory)
},
})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the initial reasoning of moving the template AFTER installing dependencies is to prevent leaving an app in a bad state if the installing fails.

For instance the cleanup and initializeGitRepository tasks wouldn't be executed in that case.

We should take into account that case, either by deleting the app folder if something fails, or with clear instructions to the user? not sure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call — switched to the "delete on failure" approach in 36deafe. If any task after the move fails (install, cleanup, or git init), the partial project at outputDirectory is removed before the error propagates, so we're back to the original "no half-baked project on disk" behavior.

let outputDirectoryCreated = false
tasks.push({
  title: 'Preparing project directory',
  task: async () => {
    await ensureAppDirectoryIsAvailable(outputDirectory, hyphenizedName)
    outputDirectoryCreated = true
    await moveFile(templateScaffoldDir, outputDirectory)
  },
})
// ...install / cleanup / git init...
try {
  await renderTasks(tasks)
} catch (error) {
  if (outputDirectoryCreated) {
    await rmdir(outputDirectory).catch(() => {})
  }
  throw error
}

The flag is set right after ensureAppDirectoryIsAvailable passes, so a partial moveFile is also cleaned up. The .catch(() => {}) on rmdir is so cleanup errors don't mask the original task error.

Copy link
Copy Markdown
Contributor

@isaacroldan isaacroldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should handle the case where dependency install fails.

If a task after the project move fails (install, cleanup, git init),
remove the half-baked scaffold at outputDirectory so users aren't left
with a directory missing node_modules / git / cleanup work. Restores
the prior behavior where a failed init left nothing behind.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: @shopify/app @shopify/app package issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants