C3: Relax empty directory check

[jculvey]

Fixes #4212.

What this PR solves / how to test:

This relaxes the existing directory check to allow files such as .git, .hg, and .DS_Store that are created by IDEs, the OS, or are benign config files. These don't conflict with c3 and the existing check impedes the workflow of users who create folders via their IDE.

Associated docs issue(s)/PR(s):

• [insert associated docs issue(s)/PR(s)]

Author has included the following, where applicable:

• Tests
• [ x ] Changeset (Changeset guidelines)

Reviewer is to perform the following, as applicable:

• Checked for inclusion of relevant tests
• Checked for inclusion of a relevant changeset
• Checked for creation of associated docs updates
• Manually pulled down the changes and spot-tested

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

[changeset-bot]

🦋 Changeset detected

Latest commit: 952c402

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
create-cloudflare	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6670449925/npm-package-wrangler-4226

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6670449925/npm-package-wrangler-4226

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6670449925/npm-package-wrangler-4226 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6670449925/npm-package-cloudflare-pages-shared-4226

Note that these links will no longer work once the GitHub Actions artifact expires.

---

wrangler@3.15.0 includes the following runtime dependencies:

Package	Constraint	Resolved
miniflare	3.20231025.0	3.20231025.0
workerd	1.20231025.0	1.20231025.0
workerd --version	1.20231025.0	2023-10-25

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

[dario-piotrowicz]

I'm really not sure about this approach, it seems rather brittle and requiring lots of maintenance tweaking in the long run 😕

for example, I can see LICENSE, what about LICENSE.md, licence.md? README? README.md? readme.md? .gitkeep? .vscode?

I think the list can go on and on and be rather long etc...

Could we instead do something like showing a warning telling the user this directory is not empty, the process might fail, do you want to proceed? Y/n? or something along those lines?

Maybe instead of listing what we allow, we could list all the files that we have in our templates and just error on conflicts for those? and let the process continue otherwise (meaning that the framework clis would do whatever checks they do for webFrameworks hopefully covering that for us?)

[jculvey]

I'm really not sure about this approach, it seems rather brittle and requiring lots of maintenance tweaking in the long run 😕

for example, I can see LICENSE, what about LICENSE.md, licence.md? README? README.md? readme.md? .gitkeep? .vscode?

I think the list can go on and on and be rather long etc...

You're not wrong 😄 . The list does seem hit or miss, and does seem like the type of thing that will need to be tweaked to no end. FWIW astro and next use the same technique in their scaffolding tools, as do others. I don't think we should maintain the list ourselves, but rather use the union of all files allowed by framework creators.

Maybe instead of listing what we allow, we could list all the files that we have in our templates and just error on conflicts for those? and let the process continue otherwise (meaning that the framework clis would do whatever checks they do for webFrameworks hopefully covering that for us?)

I think the block list turns out to be a hard problem. If we wanted to do this, we'd have to generate permutations of every possible c3 run with each option, and then collect a set of all the possible files to check against. This too will change over time, so we'd have to automate the process to happen on PRs (possibly just of new templates).

Then, if we let framework clis just do whatever checks just handle the validation, it means that instead of failing during the first step like this PR would do, they will fail during the scaffolding step as we call the framework cli.

Could we instead do something like showing a warning telling the user this directory is not empty, the process might fail, do you want to proceed? Y/n? or something along those lines?

This sounds like a better option, but I think we want to strongly avoid destructive operations. If we confirm and let them despite the file being non-empty this is possible in a way that it's not today. I think we should still crash in scenarios where we think this could happen, but just exempt a few simple corner cases where this commonly pops up for users -- like when they create a directory via an IDE or some OS utility that creates a couple hidden files.

[dario-piotrowicz]

@jculvey I see, well thanks for the reply, I'm still not convinced about this but:

• it's better then the current situation
• it doesn't seem like we have clearly better alternatives

So I am fine to proceed with this if you feel strongly about it 🙂

One minor thing (kinda alluded in my previous comment) I would suggest adding logic for having the checks case insensitive and potentially ignoring file extensions (so that you can match match LICENCE if the user has a LICENCE.md) or something similar to increase the range we can cover 🙂

[codecov]

Codecov Report

Merging #4226 (952c402) into main (7e05f38) will increase coverage by 0.04%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4226      +/-   ##
==========================================
+ Coverage   75.38%   75.43%   +0.04%     
==========================================
  Files         223      223              
  Lines       12327    12327              
  Branches     3187     3187              
==========================================
+ Hits         9293     9299       +6     
+ Misses       3034     3028       -6     

see 4 files with indirect coverage changes

[IgorMinar]

LGTM - c3 only changes

[DaniFoldi]

Hi 👋

I've been following this PR, thanks for the quick turnaround @jculvey. Am I right saying that the newest implementation fails if the directory is non-empty, but allows the user to overwrite, which deletes everything from the target directory?

If so, I'd be cautious about proceeding - this approach doesn't actually solve my main issue, rather it sets everything on fire and hides in the smoke. For the most part, there shouldn't be anything wrong deleting everything when I'm setting up a project, but what if I created a repo on GitHub that included a
LICENSEfile? Now if that's deleted, do I have to recreate? With.git` gone, do I have to clone again? What about other template files..

For the most part, I'd say there's actually nothing c3 should validate here; create-{framework} (this is for pages/website templates only) already validates for files it considers safe, and as far as my understanding goes c3 doesn't usually create new files for the Pages deployment configuration. For workers, I'd keep the whitelist like other projects do, maybe expanding by the case insensitivity or some other files, but judging by the number of issues they've had I'd say their approach is just fine.

[Cherry]

I'm not super familiar with this code, so this is a drive-by review, but this feels very dangerous to me. Someone could run this in /, or C:/ with --overwrite and then end up with a bunch of their filesystem being wiped if I'm understanding correctly?

Even with a confirmation, that feels very heavy-handed and not something I've personally experienced with other tooling.

[jculvey]

I'm going to change it back to the previous strategy of failing on non-empty directories with the exception of an allow list, but for posterity:

Someone could run this in /, or C:/ with --overwrite and then end up with a bunch of their filesystem being wiped if I'm understanding correctly?

Correct, if you ran npm create cloudflare / -- --overwrite, it would wipe the filesystem. Just as it would if you ran rm -rf /`. Does this seem like a common, realistic use case that we should account for?

Even with a confirmation, that feels very heavy-handed and not something I've personally experienced with other tooling.

Here's a couple of examples of similar tools that do the same thing:

• create-solid
• create-vue

[Cherry]

Thanks for the insight @jculvey. I think the previous strategy makes the most sense.

As for those other packages, if I'm reading them correctly from a very brief investigation:

• create-solid seems to only remove the tempTemplate dir inside your current working directory, which they define as path.join(target, ".solid-start").
• and create-vue seems to explicitly check if they enter a ., requires a --force, and even re-prompts them that they're about to remove everything in that directory, listing it back to them

[jculvey]

Am I right saying that the newest implementation fails if the directory is non-empty, but allows the user to overwrite, which deletes everything from the target directory?
If so, I'd be cautious about proceeding - this approach doesn't actually solve my main issue, rather it sets everything on fire and hides in the smoke. For the most part, there shouldn't be anything wrong deleting everything when I'm setting up a project, but what if I created a repo on GitHub that included a LICENSEfile? Now if that's deleted, do I have to recreate? With.git` gone, do I have to clone again?

That's fair. I'm going to change it back to the previous implementation of failing on non-empty directories but exempting files in an allow-list.

What about other template files..

What are other template files?

For the most part, I'd say there's actually nothing c3 should validate here; create-{framework} (this is for pages/website templates only) already validates for files it considers safe, and as far as my understanding goes c3 doesn't usually create new files for the Pages deployment configuration. For workers, I'd keep the whitelist like other projects do, maybe expanding by the case insensitivity or some other files, but judging by the number of issues they've had I'd say their approach is just fine.

I don't think we want to drop validation entirely. create-cloudflare isn't designed to be run in existing projects, and we can't provide guarantees around that experience. Each framework creator will validate what files it considers safe, but it won't check for files that could potentially conflict with pages or workers itself. Additionally, we want to protect against scenarios where the user accidentally runs c3 in an existing directory where they didn't intend to.