Continue fs-extra migration #1
Conversation
passes existing spec suite
These were using the parent of the actual target as dest
spec/create.spec.js
Outdated
| }); | ||
| afterEach(function () { | ||
| afterAll(function () { |
There was a problem hiding this comment.
Why this change and why here?
There was a problem hiding this comment.
Why: Since with emptyDirSync we empty tmpDir in beforeEach so there's no need to delete the tmpDir itself after every test, only at the very end.
Why here: Because this change is kind of enabled by the rich fs-extra API 🤷♂️
index.js
Outdated
| @@ -309,7 +309,7 @@ function copyTemplateFiles (templateDir, projectDir, isSubDir) { | |||
| // if template is a www dir | |||
| if (path.basename(templateDir) === 'www') { | |||
| copyPath = path.resolve(templateDir); | |||
| fs.copySync(copyPath, projectDir); | |||
| fs.copySync(copyPath, path.resolve(projectDir, 'www')); | |||
There was a problem hiding this comment.
I am wondering why this problem does not show up when I run the spec?
There was a problem hiding this comment.
I didn't check, but this case might not be covered or not sufficiently tested. Or I'm just plainly wrong 😉
But as far as I understand it, with copySync you always have the full file path in either argument. Whether you copy file or folder. This leads to less surprising if a bit wordier copying IMO.
index.js
Outdated
| @@ -400,7 +400,7 @@ function linkFromTemplate (templateDir, projectDir) { | |||
| // if template/www/config.xml then copy to project/config.xml | |||
| copyDst = path.join(projectDir, 'config.xml'); | |||
| if (!fs.existsSync(copyDst) && fs.existsSync(copySrc)) { | |||
| fs.copySync(copySrc, projectDir); | |||
There was a problem hiding this comment.
Ditto: I am wondering why this problem does not show up when I run the spec?
index.js
Outdated
| copyContentsIfNotExists(stockAssetPath('www'), path.join(dir, 'www')); | ||
| copyContentsIfNotExists(stockAssetPath('hooks'), path.join(dir, 'hooks')); | ||
| copyIfNotExists(stockAssetPath('www'), path.join(dir, 'www')); | ||
| copyIfNotExists(stockAssetPath('hooks'), path.join(dir, 'hooks')); |
There was a problem hiding this comment.
I wanted to make it more clear about what this function actually does? Wonder if we should just remove the separate function altogether?
There was a problem hiding this comment.
Well, the function would also work for plain files. But for that case the name makes no sense.
I'd keep it for now. It will have to be changed anyway in case some of apache/cordova-discuss#89 is accepted.
brody4hire
force-pushed
the
introduce-fs-extra-usage
branch
2 times, most recently
from
f89950f to
538a345
Compare
I left the commits small so changes can be more easily tracked. Feel free to squash after changes have been reviewed. You can add me as a Co-Author.