gh-1269: Enabling nested folder paths for project name #1270

Merged
merged 3 commits into from Dec 18, 2016

Projects

None yet

4 participants

@dinukadesilva
Contributor
dinukadesilva commented Dec 14, 2016 edited

This fixes #1269

@facebook-github-bot

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

packages/create-react-app/index.js
@@ -105,7 +105,7 @@ function createApp(name, verbose, version) {
checkAppName(appName);
if (!pathExists.sync(name)) {
- fs.mkdirSync(root);
@gaearon
gaearon Dec 14, 2016 Contributor

Can we just use mkdirpSync instead? I think that's what it would do.

@dinukadesilva
dinukadesilva Dec 14, 2016 Contributor

Ohh yes........ i'l update that.......
Thanks a lot i could learn something.

@dinukadesilva
dinukadesilva Dec 14, 2016 Contributor

@gaearon For that, we need to add https://www.npmjs.com/package/fs.extra as a dependency. Actually, I found few libraries for the same purpose (maybe this too). But, I thought of not adding since it's an additional dependency.

What do you think?

@fson
fson Dec 14, 2016 Collaborator

Let's just use the mkdirp package, because we only need this one thing, it's small and fs.extra would just include mkdirp as a dependency.

We'll be able to get rid of pathExists so the total number of dependencies is still the same :) Checking if a folder/file exists before creating it is an anti-pattern, so trying to create the directory and just checking if it existed is also better in that sense.

@gaearon
gaearon Dec 14, 2016 Contributor

Ah sorry, I thought we were already using fs-extra. Turns out we do in react-scripts, but not not in create-react-app.

@dinukadesilva
dinukadesilva Dec 14, 2016 Contributor

So, does that make sence to add fs-extra.

Even i think it would be better since there'll be more use cases where it's helpful.

@facebook-github-bot

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

packages/create-react-app/index.js
@@ -136,6 +136,16 @@ function createApp(name, verbose, version) {
run(root, appName, version, verbose, originalDirectory);
}
+function createFolderPath(path) {
+ var projectPath = (path + "/");
@gaearon
gaearon Dec 14, 2016 Contributor

This needs to work on Windows.

@dinukadesilva
dinukadesilva Dec 14, 2016 Contributor

I'm in windows. with the slash it's working fine.

Are you talking about the backslash?

@dinukadesilva
Contributor

@gaearon @fson ........ any updates about this PR?

@fson
Collaborator
fson commented Dec 15, 2016

@dinukadesilva Please use mkdirsSync (fs-extra) or mkdirP.sync (mkdirp). The code is almost identical between the two, the only difference seems to be this check in fs-extra which is to fix jprichardson/node-fs-extra#209 (doesn't seem to be handled by mkdirp at the moment).

@dinukadesilva
Contributor

@fson @gaearon
I added fs-extra and removed path-exists . Also I added few e2e test cases to verify the change.

- if (!pathExists.sync(name)) {
- fs.mkdirSync(root);
- } else if (!isSafeToCreateProjectIn(root)) {
+ fs.ensureDirSync(name);
@fson
fson Dec 18, 2016 Collaborator

Nice 👍

tasks/e2e.sh
+mkdir cc
+cd cc
+mkdir dd
+cd dd
@fson
fson Dec 18, 2016 Collaborator

You could just do mkdir -p test-app-nested-paths-t1/aa/bb/cc/dd.

@fson
fson approved these changes Dec 18, 2016 View changes

Looks good to me!

@fson fson merged commit 5fa34dd into facebookincubator:master Dec 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fson fson added this to the 0.9.0 milestone Dec 18, 2016
@stephenjwatkins stephenjwatkins added a commit to stephenjwatkins/create-react-app that referenced this pull request Jan 4, 2017
@dinukadesilva @stephenjwatkins dinukadesilva + stephenjwatkins gh-1269: Enabling nested folder paths for project name (#1270)
* gh-1269: Enabling nested folder paths for project name

* gh-1269: Added "fs-extra" and removed "path-exists"

* gh-1269: Added e2e test cases to verify nested folder names
64ecde0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment