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

Fixes issue in Meteor 1.3 release. #241

Merged
merged 5 commits into from
Sep 9, 2016
Merged

Fixes issue in Meteor 1.3 release. #241

merged 5 commits into from
Sep 9, 2016

Conversation

maka-io
Copy link
Contributor

@maka-io maka-io commented Mar 31, 2016

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

@maka-io maka-io closed this Apr 3, 2016
@maka-io
Copy link
Contributor Author

maka-io commented Apr 10, 2016

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 maka-io reopened this Apr 10, 2016
@maka-io
Copy link
Contributor Author

maka-io commented Apr 10, 2016

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
Copy link

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
Copy link
Contributor Author

maka-io commented May 12, 2016

@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
Copy link

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 [email protected] 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 #241 (comment)

@switheyw
Copy link

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 [email protected] 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 <[email protected] mailto:[email protected]> 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 #241 (comment)

@lukecampbell
Copy link

Are there any updates on this PR?

@chrisbutler
Copy link
Contributor

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

@chrisbutler chrisbutler closed this Sep 9, 2016
@chrisbutler chrisbutler reopened this Sep 9, 2016
@chrisbutler chrisbutler merged commit 680ab28 into iron-meteor:master Sep 9, 2016
@lukecampbell
Copy link

Yeah I hopped on the @maka-io wagon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants