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

feat: check for conflicting files #236

Closed
wants to merge 4 commits into from
Closed

feat: check for conflicting files #236

wants to merge 4 commits into from

Conversation

AlexanderHott
Copy link

@AlexanderHott AlexanderHott commented Jul 22, 2022 •
edited
Loading

Check for conflicting files

  • I reviewed linter warnings + errors, resolved formatting, types and other issues related to my work
  • The PR title follows the convention we established conventional-commit
  • [-] I performed a functional test on my final commit

It works fine if given a project name, but I couldn't figure out how to test it with . as the name because pnpm start . fails with

> next start "."

'next' is not recognized as an internal or external command,
operable program or batch file.

when inside a test folder.

[Short summary/description of story/feature/bugfix]
Checks if existing files will be overwritten and adds the option to overwrite files if there are conflicts.

ℹ project is not empty, but no conflicting files found, continuing...

If there are conflicts

? Warning: project already exists and has conflicting files:

  project\.env-example
  project\.eslintrc.json
  project\next-env.d.ts
  project\next.config.mjs
  project\package.json
  project\public\favicon.ico
  project\README.md
  project\env\client-env.mjs
  project\env\env-schema.mjs
  project\env\server-env.mjs
  project\pages\index.tsx
  project\pages\_app.tsx
  project\styles\globals.css
  project\tsconfig.json
  project\.gitignore

 How would you like to proceed? (Use arrow keys)
❯ Clear directory
  Abort installation
  Overwrite files (dangerous)

Screenshots

[Screenshots]
image
image

💯

@AlexanderHott
Copy link
Author

Fix for #230

@AlexanderHott AlexanderHott changed the title Feat/overwrite files feat: overwrite files Jul 22, 2022
@AlexanderHott AlexanderHott changed the title feat: overwrite files feat: check for conflicting files Jul 22, 2022
juliusmarminge
juliusmarminge reviewed Jul 22, 2022
View reviewed changes
Copy link
Member

@juliusmarminge juliusmarminge left a comment

Choose a reason for hiding this comment

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

Not available to test this today so will have another look over the weekend/next week.

I can see some edge-cases on the top of my head. E.g. user is in a git repo which is not caught but this logic. We should probably extend the initializeGit to handle this. @JacobMGEvans sent me this a while back that may be helpful: cloudflare/wrangler2 - git-client.ts

src/helpers/scaffoldProject.ts Outdated Show resolved Hide resolved
src/helpers/scaffoldProject.ts Outdated Show resolved Hide resolved
src/utils/getAllFiles.ts Outdated Show resolved Hide resolved
@juliusmarminge juliusmarminge linked an issue Jul 23, 2022 that may be closed by this pull request
feat: add ability to scaffold in non-empty directories #230
Closed
@AlexanderHott
Copy link
Author

AlexanderHott commented Jul 25, 2022 •
edited
Loading

user is in a git repo which is not caught but this logic

The git init should fail but that won't overwrite any files.

edit: formatting

juliusmarminge
juliusmarminge requested changes Aug 2, 2022
View reviewed changes
Copy link
Member

@juliusmarminge juliusmarminge left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on this review...

I believe there are a few gotchas which have to be addressed before we can merge this feature

src/helpers/scaffoldProject.ts
@@ -26,30 +27,64 @@ export const scaffoldProject = async ({
const spinner = ora(`Scaffolding in: ${projectDir}...\n`).start();

if (fs.existsSync(projectDir)) {
if (fs.readdirSync(projectDir).length === 0) {
const userFilesSet = new Set(getAllFiles(projectDir));
const srcFiles = [...getAllFiles(srcDir), ".gitignore"]; // template files
Copy link
Member

Choose a reason for hiding this comment

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

this means we are just checking against the base app without any addons. If there is files that would conflict with any of these other ones these would not be matched and potentially removed by accident

src/helpers/scaffoldProject.ts
if (fs.readdirSync(projectDir).length === 0) {
const userFilesSet = new Set(getAllFiles(projectDir));
const srcFiles = [...getAllFiles(srcDir), ".gitignore"]; // template files
const conflictingFiles = srcFiles.filter((element) =>
Copy link
Member

Choose a reason for hiding this comment

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

userFilesSet might have e.g. prisma/schema.prisma which would not be in srcFiles and therefore not in conflictingFiles. Continuing to scaffold and letting the user pick prisma would override the existing file which is not what we want.

@juliusmarminge
Copy link
Member

Closed as inactive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusmarminge juliusmarminge juliusmarminge requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: add ability to scaffold in non-empty directories
2 participants
@AlexanderHott @juliusmarminge