Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove potential leftover data #52

Merged
merged 2 commits into from
Sep 25, 2024
Merged

remove potential leftover data #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8cabe37
remove flake.lock
tatchi Sep 22, 2024
88bcdca
add Effect.ignore to the places where files are removed
tatchi Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/curvy-pianos-change.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"create-effect-app": patch
---

Removes potential leftover files
16 changes: 9 additions & 7 deletions packages/create-effect-app/src/Cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,12 +210,14 @@ function createTemplate(config: TemplateConfig) {
// Remove the .changesets directory
yield* fs.remove(path.join(config.projectName, ".changeset"), {
recursive: true
})
}).pipe(Effect.ignore)
// Remove patches for changesets
const patches = yield* fs.readDirectory(path.join(config.projectName, "patches")).pipe(
Effect.map(Array.filter((file) => file.includes("changeset")))
)
yield* Effect.forEach(patches, (patch) => fs.remove(path.join(config.projectName, "patches", patch)))
yield* Effect.forEach(patches, (patch) => fs.remove(path.join(config.projectName, "patches", patch))).pipe(
Effect.ignore
)
// Remove patched dependencies for changesets
const depsToRemove = Array.filter(
Object.keys(packageJson["pnpm"]["patchedDependencies"]),
Expand All @@ -242,22 +244,22 @@ function createTemplate(config: TemplateConfig) {
}
// If git workflows are enabled, remove changesets related workflows
if (config.projectType.withWorkflows) {
yield* fs.remove(path.join(config.projectName, ".github", "workflows", "release.yml"))
yield* fs.remove(path.join(config.projectName, ".github", "workflows", "release.yml")).pipe(Effect.ignore)
}
}

// Handle user preferences for Nix flakes
if (!config.projectType.withNixFlake) {
yield* Effect.forEach(
[".envrc", "flake.lock", "flake.nix"],
[".envrc", "flake.nix"],
Copy link
Contributor Author

@tatchi tatchi Sep 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This effect was failing because flake.lock doesn't exist, so we're not going past this code in this case. I feel like this is not the best fix; we should try to avoid this kind of situation in the future.

I guess we could pass { force: true } to fs.remove to ignore the exception if the path does not exist. Or handle it at the effect level and use something like Effect.ignore/ignoreLogged. Not sure if ignoring the error is the best solution though, open to any better idea ?

Copy link
Member

Choose a reason for hiding this comment

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

@tatchi - thank you! I think either solution is fine, but I feel like I would probably use Effect.ignore for this case.

Either way, this PR looks good for this particular issue. If you want to add Effect.ignores in the relevant places as part of this PR let me know. Otherwise also let me know if you're happy with this as is and I'll get this merged 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we basically add Effect.ignore to every place where we use fs.ignore ?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can start with just adding it to places where we attempt to remove files.

(file) => fs.remove(path.join(config.projectName, file))
)
).pipe(Effect.ignore)
}

// Handle user preferences for ESLint
if (!config.projectType.withESLint) {
// Remove eslint.config.mjs
yield* fs.remove(path.join(config.projectName, "eslint.config.mjs"))
yield* fs.remove(path.join(config.projectName, "eslint.config.mjs")).pipe(Effect.ignore)
// Remove eslint dependencies
const eslintDeps = Array.filter(
Object.keys(packageJson["devDependencies"]),
Expand Down Expand Up @@ -289,7 +291,7 @@ function createTemplate(config: TemplateConfig) {
// Remove the .github directory
yield* fs.remove(path.join(config.projectName, ".github"), {
recursive: true
})
}).pipe(Effect.ignore)
}

// Write out the updated package.json
Expand Down