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

Overwrite Option #21

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

marwanhilmi
Copy link

Hey @maxogden,

I created this as potential fix to #20. By default overwrite is true but we can toggle it off. I created a function to remove the symlink if it already exists. I added some tests but I did run into a weird issue with the last test:

test('callback called once', function (t) {
  rimraf.sync(targetC)
  t.plan(2)
  console.log('extracting to', targetC)
  extract(sourceC, {dir: targetC}, function (err) {
    if (err) throw err

    // this triggers an error due to symlink creation
    // NOTE the new overwrite: false option
    extract(sourceC, {dir: targetC, overwrite: false}, function (err) {
      if (err) t.ok(true, 'error passed')
      t.ok(true, 'callback called')
    })
  })
})

It craps out with:

events.js:141
      throw er; // Unhandled 'error' event
      ^
Error: EBADF: bad file descriptor, read
    at Error (native)

Any ideas?
Looks like this unresolved node bug: nodejs/node#2955 && nodejs/node#2950

function overwriteExistingSymlink () {
fs.unlink(dest, function (err) {
if (err) {
if (err.code === 'ENOENT') return writeSymlink()
Copy link
Author

Choose a reason for hiding this comment

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

Instead of doing a path exists check before attempting to delete, I just try to delete and if there's a ENOENT, we just proceed to write the symlink.

@max-mapper
Copy link
Owner

Crazy, haven't run into this case before. cc'ing @mafintosh here too

@unindented
Copy link

@maxogden any intent to merge the PR?

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.

3 participants