iron-meteor / iron-cli

A scaffolding command line tool for Meteor applications.
640 stars 82 forks source link

Fixes issue in Meteor 1.3 release. #241

Closed maka-io closed 8 years ago

maka-io commented 8 years ago

For some reason, deleting a directory with fs is bugged. Tried this delete package and it worked great.

maka-io commented 8 years ago

I closed this because I saw an issue and wanted to address it. I updated delete to be sync as was being used by fs.

maka-io commented 8 years ago

I should also mention that I did a

npm cache clean

at some point during testing. If this looks like it's creating the files but actually doesn't, try a cache clean and then try again.

switheyw commented 8 years ago

Which package is 'this delete package', where can I find it?
Is it window's specific? Otherwise I assume that the delete call copes with both files and directories. The recursive routine, using the standard fs package calls both fs.unlink and fs.rmdir (sync). Do you think you are making the right move by abandoning the stock standard file system routines?

maka-io commented 8 years ago

@switheyw , you can get the delete package here: delete, or

npm install delete

It's not windows specific, I've been using it on OS X, Ubuntu and Windows. I do favor fs when it works in the application and I believe it's the right call in order to keep the application working. Removing the dependency on delete by finding a solution to the issue with fs would be awesome.

switheyw commented 8 years ago

You don’t agree with my original diagnosis? The code tries to delete files and directories with fs.unlink when in reality fs.unlink removes files and fs.rmdir removes empty directories. Why this turned up in Meteor 1.3 I do not know.

If you instrument your iron create, do you show both files and directories being removed or just files?

regards Stephen

On May 12, 2016, at 6:55 AM, Matt Campbell notifications@github.com wrote:

@switheyw https://github.com/switheyw , you can get the delete package here: delete https://www.npmjs.com/package/delete, or

npm install delete It's not windows specific, I've been using it on OS X, Ubuntu and Windows. I do favor fs when it works in the application and I believe it's the right call in order to keep the application working. Removing the dependency on delete by finding a solution to the issue with fs would be awesome.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/iron-meteor/iron-cli/pull/241#issuecomment-218724479

switheyw commented 8 years ago

Thanks for getting back to me - I just looked at delete, and it does indeed delete directory trees - files and dirs.

Here is “my” fix. Somebody else’s recursive delete (I think I included his name in one of my posts) and the if else test in original module below. Perhaps 1.3 has better or more error checking and we never knew that the the directory itself was left behind?

// Try this instead of fs.unlink var deleteFolderRecursive = function(path) { if( fs.existsSync(path) ) { fs.readdirSync(path).forEach(function(file,index){ var curPath = path + "/" + file; if(fs.lstatSync(curPath).isDirectory()) { // recurse deleteFolderRecursive(curPath); } else { // delete file fs.unlinkSync(curPath); } }); fs.rmdirSync(path); }

try { var spinHandle = this.logWithSpinner('Creating project ', name); var appDirectory = path.join(opts.cwd, name); this.execSync('meteor create ' + name, {cwd: opts.cwd, silent: true}); _.each(fs.readdirSync(appDirectory), function (entryPath) { if (entryPath === '.git') return; if (entryPath === '.meteor') return; var fullpath = path.join(appDirectory, entryPath);

  if(fs.lstatSync(fullpath).isDirectory()) { // recurse
    deleteFolderRecursive(fullpath);
  } else { // delete file
    fs.unlinkSync(fullpath);
  }

  // depreciate fs and use del instead.
  // this call errors out.
  //fs.unlinkSync(path.join(appDirectory, entryPath));
  //fs.unlink(path.join(appDirectory, entryPath));

On May 12, 2016, at 8:15 PM, Stephen Wright stephen.withey.wright@gmail.com wrote:

You don’t agree with my original diagnosis? The code tries to delete files and directories with fs.unlink when in reality fs.unlink removes files and fs.rmdir removes empty directories. Why this turned up in Meteor 1.3 I do not know.

If you instrument your iron create, do you show both files and directories being removed or just files?

regards Stephen

On May 12, 2016, at 6:55 AM, Matt Campbell <notifications@github.com mailto:notifications@github.com> wrote:

@switheyw https://github.com/switheyw , you can get the delete package here: delete https://www.npmjs.com/package/delete, or

npm install delete It's not windows specific, I've been using it on OS X, Ubuntu and Windows. I do favor fs when it works in the application and I believe it's the right call in order to keep the application working. Removing the dependency on delete by finding a solution to the issue with fs would be awesome.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/iron-meteor/iron-cli/pull/241#issuecomment-218724479

lukecampbell commented 8 years ago

Are there any updates on this PR?

chrisbutler commented 8 years ago

@switheyw your solution does work, but i think @maka-io's use of delete is cleaner. merging to release in 1.5.3

lukecampbell commented 8 years ago

Yeah I hopped on the @maka-io wagon.