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.

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

Recursively copy the template folder #74

Merged
merged 2 commits into from
Jul 22, 2016
Merged

Recursively copy the template folder #74

merged 2 commits into from
Jul 22, 2016

Conversation

eanplatter
Copy link
Contributor

I noticed my work on #33 would only copy first in the template directory, and explicitly had to check the src folder and copy it.

This instead just recursively copies the template folder into the root of the app.

Should work with deep nested paths like ./template/tests/components/myComponentTest.js

Though we don't necessarily need it now, if someone wants to add something to the template in the future, this is probably a better way to handle copying.

@ghost ghost added the CLA Signed label Jul 22, 2016
return fs.writeFileSync(dest, fs.readFileSync(src));
function copyFileSync(src, dest) {
var target = dest;
if (fs.existsSync(dest)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is never going to happen right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the file is never going to exist? Yeah I guess not. Updating...

@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

Is there a Node Module(tm) that wraps fs API to be higher level?
Ideally we shouldn’t write it here if it exists.

@vjeux
Copy link
Contributor

vjeux commented Jul 22, 2016

Thanks for doing that. I really hate that the node fs api doesn't has this by default

@vjeux
Copy link
Contributor

vjeux commented Jul 22, 2016

@gaearon fs-extra

@eanplatter
Copy link
Contributor Author

I can instead implement fs.extra's copyRecursive method, it probably deals with cross platform issues better than a hand rolled solution. I'll give it a whirl!

@ghost ghost added the CLA Signed label Jul 22, 2016
fs.readdirSync(path.join(selfPath, 'template')).forEach(function(filename) {
if (fs.lstatSync(path.join(selfPath, 'template', filename)).isDirectory()) {
return
fs.copyRecursive(path.join(selfPath, 'template'), hostPath, function(err) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Geez, this was ridiculously easy. thanks @vjeux

Copy link
Contributor

Choose a reason for hiding this comment

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

I know. I'm super angry at node for not providing this by default. Seems like a no-brainer :(

@ghost ghost added the CLA Signed label Jul 22, 2016
@@ -56,6 +56,8 @@
},
"devDependencies": {
"bundle-deps": "^1.0.0",
"fs-extra": "^0.30.0",
"fs.extra": "^1.3.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

@eanplatter
Copy link
Contributor Author

Well, I learned a lot from this pr and #33 where I wrestled with this issue, only to find that 1 line of code could have solved it all. Worth it :)

Now that we're using the fs-extra library perhaps there are other places where we can clean the code up a little bit.

@vjeux vjeux merged commit ad2550f into facebook:master Jul 22, 2016
@vjeux
Copy link
Contributor

vjeux commented Jul 22, 2016

/me excited :)

@vjeux
Copy link
Contributor

vjeux commented Jul 22, 2016

This seems to be breaking the end to end test "Error: Cannot find module 'fs-extra'". Can you reproduce locally?

@vjeux
Copy link
Contributor

vjeux commented Jul 22, 2016

I cleared the travis node modules cache and it didn't help

@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants