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 fs-extra on linux #520

Merged
merged 1 commit into from Nov 18, 2017
Merged

Fixes fs-extra on linux #520

merged 1 commit into from Nov 18, 2017

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Nov 17, 2017

The lchown function is not available on some systems. Using fs-extra there causes a crash:

TypeError: Cannot read property 'name' of undefined
  
  at Object.<anonymous>.exports.fromCallback (../node_modules/universalify/index.js:16:26)
  at Object.<anonymous>.api.forEach.method (../node_modules/fs-extra/lib/fs/index.js:50:21)
      at Array.forEach (<anonymous>)

This PR makes sure that all commands exist before trying to convert them.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 87.364% when pulling 407b4ef on arcanis:patch-1 into a6e8cd6 on jprichardson:master.

Repository owner deleted a comment from coveralls Nov 17, 2017
@RyanZim
Copy link
Collaborator

RyanZim commented Nov 17, 2017

@arcanis Thanks for the PR. What kind of Linux are you running when you get this error? What filesystem? I'm on Linux myself, but I've never had this problem, though I can see it could happen, looking at the Node.js code.

lib/fs/index.js Outdated
// Some commands are not available on some systems. Ex:
// fs.copyFile was added in Node.js v8.5.0
// fs.mkdtemp was added in Node.js v5.10.0
// fs.lchown is not available on Linux
Copy link
Collaborator

Choose a reason for hiding this comment

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

// fs.lchown is not available on some computers

It isn't unavailable on all Linux.

lib/fs/index.js Outdated
// fs.copyFile was added in Node.js v8.5.0
// fs.mkdtemp was added in Node.js v5.10.0
// fs.lchown is not available on Linux
return fs[key]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for consistency with the previous code, to prevent possible regressions, can we change this to

return typeof fs[key] === 'function'

?

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 17, 2017

Also, please squash into one commit.

@arcanis
Copy link
Contributor Author

arcanis commented Nov 17, 2017

I'm not too sure. I think O_SYMLINK is supposed to only be present on BSD-based systems, at least according to nodejs/node-v0.x-archive#3598 and nodejs/node-v0.x-archive#2142 (comment). Will make the requested changes.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 87.364% when pulling d21da93 on arcanis:patch-1 into a6e8cd6 on jprichardson:master.

Repository owner deleted a comment from coveralls Nov 18, 2017
@RyanZim RyanZim merged commit f934357 into jprichardson:master Nov 18, 2017
@RyanZim
Copy link
Collaborator

RyanZim commented Nov 18, 2017

@arcanis Thanks!

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.

None yet

5 participants